Re: [PATCH] Staging: pi433: fix brace coding style issues in pi433_if.c

From: Marek Tomas
Date: Sun Dec 03 2017 - 04:35:14 EST

Next message: kbuild test robot: "Re: [PATCH net-next v4 2/2] net: ethernet: socionext: add AVE ethernet driver"
Previous message: santosh.shilimkar@xxxxxxxxxx: "Re: [RESEND PATCH 2/4] ARM: dts: keystone: Add generic compatible string for I2C EEPROM"
In reply to: Joe Perches: "Re: [PATCH] Staging: pi433: fix brace coding style issues in pi433_if.c"
Next in thread: Dan Carpenter: "Re: [PATCH] Staging: pi433: fix brace coding style issues in pi433_if.c"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 12/02/2017 10:50 PM, Joe Perches wrote:
> On Sat, 2017-12-02 at 22:05 +0100, Tomas Marek wrote:
>> This patch fix several brace on next line, braces not necessary, space
>> around =/<, and space before/after open/close parenthesis coding style
>> errors find by checkpatch in pi433_if.c.
>>
>> Signed-off-by: Tomas Marek <marek_tomas@xxxxxxxxxx>
>> ---
>> drivers/staging/pi433/pi433_if.c | 130 ++++++++++++---------------------------
>> 1 file changed, 40 insertions(+), 90 deletions(-)
>>
>> diff --git a/drivers/staging/pi433/pi433_if.c b/drivers/staging/pi433/pi433_if.c
>> index 2a205c6..795ae36 100644
>> --- a/drivers/staging/pi433/pi433_if.c
>> +++ b/drivers/staging/pi433/pi433_if.c
>> @@ -133,19 +133,14 @@ 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_PacketSent) {
>> 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_PayloadReady) {
>> dev_dbg(device->dev, "DIO0 irq: PayloadReady\n");
>> device->free_in_fifo = 0;
>> wake_up_interruptible(&device->fifo_wait_queue);
> I suggest using a switch/case and presumably the
> dev_dbg should be dev_dbg_ratelimited.
>
> Perhaps:
>
> {
> const char *msg = NULL;
> struct wait_queue_head *wqh;
>
> switch (device->irq_state[DIO0]) {
> case DIO_PacketSent:
> device_free_in_fifo = FIFO_SIZE;
> wqh = &device->fifo_wait_queue;
> msg = "Packet sent";
> break;
> case DIO_PayloadReady:
> device_free_in_fifo = 0;
> wqh = &device->fifo_wait_queue;
> msg = "PayloadReady";
> break;
> case DIO_Rssi_DIO0:
> wqh = &device->rx_wait_queue;
> msg = "RSSI level over
> threshold";
> break;
> }
>
> if (msg) {
> dev_dbg_ratelimited(device->dev, "DIO0 irq: %s\n", msg);
> wake_up_interruptible(whq);
> }
>
> return IRQ_HANDLED;
> }
Thanks for the input. Initially, I plan just a simple fix of errors reported by checkpatch to get familiar with patch submit procedure. Anyway, proposed change seems to be reasonable and I will try to implement it (and take advantage to send also v2 patch).

Would you like to be noted in v2 as suggested-by?

v2 coming up soon

Thank you again

Tomas
>
>> @@ -158,12 +153,9 @@ 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_FifoNotEmpty_DIO1) {
>> device->free_in_fifo = FIFO_SIZE;
>> - }
>> - else if (device->irq_state[DIO1] == DIO_FifoLevel)
>> - {
>> + } else if (device->irq_state[DIO1] == DIO_FifoLevel) {
>> if (device->rx_active) device->free_in_fifo = FIFO_THRESHOLD - 1;
>> else device->free_in_fifo = FIFO_SIZE - FIFO_THRESHOLD - 1;
>> }
>> @@ -198,12 +190,9 @@ static irqreturn_t DIO1_irq_handler(int irq, void *dev_id)
>> /* 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 == optionOn) {
>> SET_CHECKED(rf69_set_fifo_fill_condition(dev->spi, afterSyncInterrupt));
>> - }
>> - else
>> - {
>> + } else {
>> SET_CHECKED(rf69_set_fifo_fill_condition(dev->spi, always));
>> }
>> if (rx_cfg->enable_length_byte == optionOn) {
>> @@ -220,29 +209,22 @@ static irqreturn_t DIO1_irq_handler(int irq, void *dev_id)
>>
>> /* 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 == optionOn) {
>> SET_CHECKED(rf69_set_payload_length(dev->spi, 0xff));
>> - }
>> - else if (rx_cfg->fixed_message_length != 0)
>> - {
>> + } 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_address_filtering != filteringOff) payload_length++;
>> SET_CHECKED(rf69_set_payload_length(dev->spi, payload_length));
>> - }
>> - else
>> - {
>> + } else {
>> SET_CHECKED(rf69_set_payload_length(dev->spi, 0));
>> }
>>
>> /* values */
>> if (rx_cfg->enable_sync == optionOn)
>> - {
>> SET_CHECKED(rf69_set_sync_values(dev->spi, rx_cfg->sync_pattern));
>> - }
>> - if (rx_cfg->enable_address_filtering != filteringOff)
>> - {
>> +
>> + if (rx_cfg->enable_address_filtering != filteringOff) {
>> SET_CHECKED(rf69_set_node_address (dev->spi, rx_cfg->node_address));
>> SET_CHECKED(rf69_set_broadcast_address(dev->spi, rx_cfg->broadcast_address));
>> }
>> @@ -265,13 +247,10 @@ static irqreturn_t DIO1_irq_handler(int irq, void *dev_id)
>>
>> /* packet format enable */
>> if (tx_cfg->enable_preamble == optionOn)
>> - {
>> SET_CHECKED(rf69_set_preamble_length(dev->spi, tx_cfg->preamble_length));
>> - }
>> else
>> - {
>> 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) {
>> ret = rf69_set_packet_format(dev->spi, packetLengthVar);
>> @@ -341,8 +320,8 @@ static irqreturn_t DIO1_irq_handler(int irq, void *dev_id)
>> /* wait for any tx to finish */
>> dev_dbg(dev->dev,"rx: going to wait for any tx to finish");
>> retval = wait_event_interruptible(dev->rx_wait_queue, !dev->tx_active);
>> - if(retval) /* wait was interrupted */
>> - {
>> + /* wait was interrupted */
>> + if (retval) {
>> dev->interrupt_rx_allowed = true;
>> wake_up_interruptible(&dev->tx_wait_queue);
>> return retval;
>> @@ -359,8 +338,7 @@ static irqreturn_t DIO1_irq_handler(int irq, void *dev_id)
>> return retval;
>>
>> /* now check RSSI, if low wait for getting high (RSSI interrupt) */
>> - while ( !rf69_get_flag(dev->spi, rssiExceededThreshold) )
>> - {
>> + while (!rf69_get_flag(dev->spi, rssiExceededThreshold)) {
>> /* allow tx to interrupt us while waiting for high RSSI */
>> dev->interrupt_rx_allowed = true;
>> wake_up_interruptible(&dev->tx_wait_queue);
>> @@ -383,25 +361,20 @@ static irqreturn_t DIO1_irq_handler(int irq, void *dev_id)
>> irq_set_irq_type(dev->irq_num[DIO0], IRQ_TYPE_EDGE_RISING);
>>
>> /* fixed or unlimited length? */
>> - if (dev->rx_cfg.fixed_message_length != 0)
>> - {
>> - if (dev->rx_cfg.fixed_message_length > dev->rx_buffer_size)
>> - {
>> + if (dev->rx_cfg.fixed_message_length != 0) {
>> + if (dev->rx_cfg.fixed_message_length > dev->rx_buffer_size) {
>> retval = -1;
>> goto abort;
>> }
>> bytes_total = dev->rx_cfg.fixed_message_length;
>> dev_dbg(dev->dev,"rx: msg len set to %d by fixed length", bytes_total);
>> - }
>> - else
>> - {
>> + } else {
>> bytes_total = dev->rx_buffer_size;
>> dev_dbg(dev->dev, "rx: msg len set to %d as requested by read", bytes_total);
>> }
>>
>> /* length byte enabled? */
>> - if (dev->rx_cfg.enable_length_byte == optionOn)
>> - {
>> + if (dev->rx_cfg.enable_length_byte == optionOn) {
>> retval = wait_event_interruptible(dev->fifo_wait_queue,
>> dev->free_in_fifo < FIFO_SIZE);
>> if (retval) goto abort; /* wait was interrupted */
>> @@ -416,8 +389,7 @@ static irqreturn_t DIO1_irq_handler(int irq, void *dev_id)
>> }
>>
>> /* address byte enabled? */
>> - if (dev->rx_cfg.enable_address_filtering != filteringOff)
>> - {
>> + if (dev->rx_cfg.enable_address_filtering != filteringOff) {
>> u8 dummy;
>>
>> bytes_total--;
>> @@ -432,10 +404,8 @@ static irqreturn_t DIO1_irq_handler(int irq, void *dev_id)
>> }
>>
>> /* get payload */
>> - while (dev->rx_position < bytes_total)
>> - {
>> - if ( !rf69_get_flag(dev->spi, payloadReady) )
>> - {
>> + while (dev->rx_position < bytes_total) {
>> + if (!rf69_get_flag(dev->spi, payloadReady)) {
>> retval = wait_event_interruptible(dev->fifo_wait_queue,
>> dev->free_in_fifo < FIFO_SIZE);
>> if (retval) goto abort; /* wait was interrupted */
>> @@ -489,8 +459,7 @@ static irqreturn_t DIO1_irq_handler(int irq, void *dev_id)
>> int position, repetitions;
>> int retval;
>>
>> - while (1)
>> - {
>> + while (1) {
>> /* wait for fifo to be populated or for request to terminate*/
>> dev_dbg(device->dev, "thread: going to wait for new messages");
>> wait_event_interruptible(device->tx_wait_queue,
>> @@ -565,8 +534,7 @@ static irqreturn_t DIO1_irq_handler(int irq, void *dev_id)
>> disable_irq(device->irq_num[DIO0]);
>> device->tx_active = true;
>>
>> - if (device->rx_active && rx_interrupted == false)
>> - {
>> + if (device->rx_active && rx_interrupted == false) {
>> /* rx is currently waiting for a telegram;
>> * we need to set the radio module to standby
>> */
>> @@ -578,13 +546,9 @@ static irqreturn_t DIO1_irq_handler(int irq, void *dev_id)
>> 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)
>> - {
>> SET_CHECKED(rf69_set_payload_length(spi, size * tx_cfg.repetitions));
>> - }
>> else
>> - {
>> SET_CHECKED(rf69_set_payload_length(spi, 0));
>> - }
>>
>> /* configure the rf chip */
>> rf69_set_tx_cfg(device, &tx_cfg);
>> @@ -607,19 +571,17 @@ static irqreturn_t DIO1_irq_handler(int irq, void *dev_id)
>> device->free_in_fifo = FIFO_SIZE;
>> position = 0;
>> repetitions = tx_cfg.repetitions;
>> - while( (repetitions > 0) && (size > position) )
>> - {
>> - if ( (size - position) > device->free_in_fifo)
>> - { /* msg to big for fifo - take a part */
>> + while ((repetitions > 0) && (size > position)) {
>> + if ((size - position) > device->free_in_fifo) {
>> + /* msg to big for fifo - take a part */
>> int temp = device->free_in_fifo;
>> device->free_in_fifo = 0;
>> rf69_write_fifo(spi,
>> &buffer[position],
>> temp);
>> position +=temp;
>> - }
>> - else
>> - { /* msg fits into fifo - take all */
>> + } else {
>> + /* msg fits into fifo - take all */
>> device->free_in_fifo -= size;
>> repetitions--;
>> rf69_write_fifo(spi,
>> @@ -648,8 +610,7 @@ static irqreturn_t DIO1_irq_handler(int irq, void *dev_id)
>> /* everything sent? */
>> if (kfifo_is_empty(&device->tx_fifo)) {
>> abort:
>> - if (rx_interrupted)
>> - {
>> + if (rx_interrupted) {
>> rx_interrupted = false;
>> pi433_start_rx(device);
>> }
>> @@ -678,13 +639,10 @@ static irqreturn_t DIO1_irq_handler(int irq, void *dev_id)
>>
>> /* just one read request at a time */
>> mutex_lock(&device->rx_lock);
>> - if (device->rx_active)
>> - {
>> + if (device->rx_active) {
>> mutex_unlock(&device->rx_lock);
>> return -EAGAIN;
>> - }
>> - else
>> - {
>> + } else {
>> device->rx_active = true;
>> mutex_unlock(&device->rx_lock);
>> }
>> @@ -909,8 +867,7 @@ static int setup_GPIOs(struct pi433_device *device)
>> DIO1_irq_handler
>> };
>>
>> - for (i=0; i<NUM_DIO; i++)
>> - {
>> + for (i = 0; i < NUM_DIO; i++) {
>> /* "construct" name and get the gpio descriptor */
>> snprintf(name, sizeof(name), "DIO%d", i);
>> device->gpiod[i] = gpiod_get(&device->spi->dev, name, 0 /*GPIOD_IN*/);
>> @@ -923,12 +880,10 @@ static int setup_GPIOs(struct pi433_device *device)
>> if (device->gpiod[i] == ERR_PTR(-EBUSY))
>> dev_dbg(&device->spi->dev, "%s is busy.", name);
>>
>> - if ( IS_ERR(device->gpiod[i]) )
>> - {
>> + if (IS_ERR(device->gpiod[i])) {
>> retval = PTR_ERR(device->gpiod[i]);
>> /* release already allocated gpios */
>> - for (i--; i>=0; i--)
>> - {
>> + for (i--; i >= 0; i--) {
>> free_irq(device->irq_num[i], device);
>> gpiod_put(device->gpiod[i]);
>> }
>> @@ -967,8 +922,7 @@ static void free_GPIOs(struct pi433_device *device)
>> {
>> int i;
>>
>> - for (i=0; i<NUM_DIO; i++)
>> - {
>> + for (i = 0; i < NUM_DIO; i++) {
>> /* check if gpiod is valid */
>> if ( IS_ERR(device->gpiod[i]) )
>> continue;
>> @@ -1032,13 +986,10 @@ static int pi433_probe(struct spi_device *spi)
>> /* spi->max_speed_hz = 10000000; 1MHz already set by device tree overlay */
>>
>> retval = spi_setup(spi);
>> - if (retval)
>> - {
>> + if (retval) {
>> dev_dbg(&spi->dev, "configuration of SPI interface failed!\n");
>> return retval;
>> - }
>> - else
>> - {
>> + } else {
>> dev_dbg(&spi->dev,
>> "spi interface setup: mode 0x%2x, %d bits per word, %dhz max speed",
>> spi->mode, spi->bits_per_word, spi->max_speed_hz);
>> @@ -1124,8 +1075,7 @@ static int pi433_probe(struct spi_device *spi)
>> pr_err("pi433: device register failed\n");
>> retval = PTR_ERR(device->dev);
>> goto device_create_failed;
>> - }
>> - else {
>> + } else {
>> dev_dbg(device->dev,
>> "created device for major %d, minor %d\n",
>> MAJOR(pi433_dev),

Next message: kbuild test robot: "Re: [PATCH net-next v4 2/2] net: ethernet: socionext: add AVE ethernet driver"
Previous message: santosh.shilimkar@xxxxxxxxxx: "Re: [RESEND PATCH 2/4] ARM: dts: keystone: Add generic compatible string for I2C EEPROM"
In reply to: Joe Perches: "Re: [PATCH] Staging: pi433: fix brace coding style issues in pi433_if.c"
Next in thread: Dan Carpenter: "Re: [PATCH] Staging: pi433: fix brace coding style issues in pi433_if.c"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]