You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@nuttx.apache.org by GitBox <gi...@apache.org> on 2020/08/26 06:01:19 UTC
[GitHub] [incubator-nuttx] raiden00pl commented on a change in pull request #1646: nRF52: various fixes
raiden00pl commented on a change in pull request #1646:
URL: https://github.com/apache/incubator-nuttx/pull/1646#discussion_r477049177
##########
File path: arch/arm/src/nrf52/hardware/nrf52_gpiote.h
##########
@@ -78,26 +63,30 @@
/* INTENSET/INTENCLR Register */
#define GPIOTE_INT_IN_SHIFT 0 /* Bits 0-7: Enable interrupt for event IN[i] */
+
#define GPIOTE_INT_IN_MASK (0xff << GPIOTE_INT_IN_SHIFT)
# define GPIOTE_INT_IN(i) ((1 << (i + GPIOTE_INT_IN_SHIFT)) & GPIOTE_INT_IN_MASK)
+
#define GPIOTE_INT_PORT 31 /* Bit 31: Enable interrupt for event PORT */
/* CONFIG Register */
-#define GPIOTE_CONFIG_MODE_SHIFT 0 /* Bits 0-1: Mode */
-#define GPIOTE_CONFIG_MODE_MASK (0x3 << GPIOTE_CONFIG_MODE_SHIFT)
-# define GPIOTE_CONFIG_MODE_DIS (0x0 << GPIOTE_CONFIG_MODE_SHIFT) /* 0: Disabled */
-# define GPIOTE_CONFIG_MODE_EV (0x1 << GPIOTE_CONFIG_MODE_SHIFT) /* 1: Event */
-# define GPIOTE_CONFIG_MODE_TS (0x3 << GPIOTE_CONFIG_MODE_SHIFT) /* 2: Task */
-#define GPIOTE_CONFIG_PSEL_SHIFT (8) /* Bits 8-12: GPIO number */
-#define GPIOTE_CONFIG_PSEL_MASK (0x1f << GPIOTE_CONFIG_PSEL_SHIFT)
-#define GPIOTE_CONFIG_PORT_SHIFT (13) /* Bit 13: GPIO port */
-#define GPIOTE_CONFIG_POL_SHIFT (16) /* Bits 16-17: Polarity */
-#define GPIOTE_CONFIG_POL_MASK (0x3 << GPIOTE_CONFIG_POL_SHIFT)
-# define GPIOTE_CONFIG_POL_NONE (0x0 << GPIOTE_CONFIG_POL_SHIFT) /* 0: None */
-# define GPIOTE_CONFIG_POL_LTH (0x1 << GPIOTE_CONFIG_POL_SHIFT) /* 1: LoToHi */
-# define GPIOTE_CONFIG_POL_HTL (0x2 << GPIOTE_CONFIG_POL_SHIFT) /* 2: HiToLo */
-# define GPIOTE_CONFIG_POL_TG (0x3 << GPIOTE_CONFIG_POL_SHIFT) /* 3: Toggle */
-#define GPIOTE_CONFIG_OUTINIT (20) /* Bit 20: Initial value */
+#define GPIOTE_CONFIG_MODE_SHIFT 0 /* Bits 0-1: Mode */
Review comment:
What was wrong with that? This block was aligned with the previous section, now is not.
##########
File path: arch/arm/src/nrf52/hardware/nrf52_radio.h
##########
@@ -436,11 +423,18 @@
#define RADIO_MODE_MASK (0xf << RADIO_MODE_SHIFT)
#define RADIO_MODE_NRF1MBIT (0x00 << RADIO_MODE_SHIFT) /* 0: 1 Mbit/s Nordic proprietary radio mode */
#define RADIO_MODE_NRF2MBIT (0x01 << RADIO_MODE_SHIFT) /* 1: 2 Mbit/s Nordic proprietary radio mode */
+
+#if defined(CONFIG_ARCH_CHIP_NRF52832)
+#define RADIO_MODE_NRF250KBIT (0x02 << RADIO_MODE_SHIFT) /* 2: 250 kbit/s Nordic proprietary radio mode (deprecated) */
+#define RADIO_MODE_BLE1MBIT (0x03 << RADIO_MODE_SHIFT) /* 3: 1 Mbit/s BLE */
+#define RADIO_MODE_BLE2MBIT (0x04 << RADIO_MODE_SHIFT) /* 4: 2 Mbit/s BLE */
+#elif defined(CONFIG_ARCH_CHIP_NRF52840)
#define RADIO_MODE_BLE1MBIT (0x03 << RADIO_MODE_SHIFT) /* 3: 1 Mbit/s BLE */
#define RADIO_MODE_BLE2MBIT (0x04 << RADIO_MODE_SHIFT) /* 4: 2 Mbit/s BLE */
#define RADIO_MODE_BLELR125KBIT (0x05 << RADIO_MODE_SHIFT) /* 5: Long range 125 kbit/s TX, 125 kbit/s and 500 kbit/s RX */
#define RADIO_MODE_BLELR500KBIT (0x06 << RADIO_MODE_SHIFT) /* 6: Long range 500 kbit/s TX, 125 kbit/s and 500 kbit/s RX */
#define RADIO_MODE_IEEE802154 (0x0f << RADIO_MODE_SHIFT) /* 15: IEEE 802.15.4-2006 250 kbit/s */
+#endif
Review comment:
```suggestion
#if defined(CONFIG_ARCH_CHIP_NRF52832)
# define RADIO_MODE_NRF250KBIT (0x02 << RADIO_MODE_SHIFT) /* 2: 250 kbit/s Nordic proprietary radio mode (deprecated) */
# define RADIO_MODE_BLE1MBIT (0x03 << RADIO_MODE_SHIFT) /* 3: 1 Mbit/s BLE */
# define RADIO_MODE_BLE2MBIT (0x04 << RADIO_MODE_SHIFT) /* 4: 2 Mbit/s BLE */
#elif defined(CONFIG_ARCH_CHIP_NRF52840)
# define RADIO_MODE_BLE1MBIT (0x03 << RADIO_MODE_SHIFT) /* 3: 1 Mbit/s BLE */
# define RADIO_MODE_BLE2MBIT (0x04 << RADIO_MODE_SHIFT) /* 4: 2 Mbit/s BLE */
# define RADIO_MODE_BLELR125KBIT (0x05 << RADIO_MODE_SHIFT) /* 5: Long range 125 kbit/s TX, 125 kbit/s and 500 kbit/s RX */
# define RADIO_MODE_BLELR500KBIT (0x06 << RADIO_MODE_SHIFT) /* 6: Long range 500 kbit/s TX, 125 kbit/s and 500 kbit/s RX */
# define RADIO_MODE_IEEE802154 (0x0f << RADIO_MODE_SHIFT) /* 15: IEEE 802.15.4-2006 250 kbit/s */
#endif
```
##########
File path: arch/arm/src/nrf52/hardware/nrf52_radio.h
##########
@@ -454,20 +448,27 @@
#define RADIO_PCNF0_S1LEN_MASK (0xf << RADIO_PCNF0_S1LEN_SHIFT)
#define RADIO_PCNF0_S1LEN_MAX (0xf)
#define RADIO_PCNF0_S1INCL (1 << 20) /* Bit 20: Include or exclude S1 field in RAM */
+
+#ifdef CONFIG_ARCH_CHIP_NRF52840
#define RADIO_PCNF0_CILEN_SHIFT (22) /* Bits 22-23: Length of code indicator - long range */
#define RADIO_PCNF0_CILEN_MASK (0x3 << RADIO_PCNF0_CILEN_SHIFT)
#define RADIO_PCNF0_CILEN_MAX (0x3)
+#endif
+
#define RADIO_PCNF0_PLEN_SHIFT (24) /* Bits 24-25: Length of preamble on air */
#define RADIO_PCNF0_PLEN_MASK (0x3 << RADIO_PCNF0_PLEN_SHIFT)
# define RADIO_PCNF0_PLEN_8BIT (0 << RADIO_PCNF0_PLEN_SHIFT)
# define RADIO_PCNF0_PLEN_16BIT (1 << RADIO_PCNF0_PLEN_SHIFT)
# define RADIO_PCNF0_PLEN_32BITZ (2 << RADIO_PCNF0_PLEN_SHIFT)
# define RADIO_PCNF0_PLEN_LONGRANGE (3 << RADIO_PCNF0_PLEN_SHIFT)
+
+#ifdef CONFIG_ARCH_CHIP_NRF52840
#define RADIO_PCNF0_CRCINC_SHIFT (26) /* Bit 26: Indicates if LENGTH field contains CRC */
#define RADIO_PCNF0_CRCINC (1 << RADIO_PCNF0_CRCINC_SHIFT)
#define RADIO_PCNF0_TERMLEN_SHIFT (29) /* Bits 29-30: Length of TERM field in Long Range operation */
#define RADIO_PCNF0_TERMLEN_MASK (0x3 << RADIO_PCNF0_TERMLEN_SHIFT)
#define RADIO_PCNF0_TERMLEN_MAX (0x3)
Review comment:
```suggestion
# define RADIO_PCNF0_CRCINC_SHIFT (26) /* Bit 26: Indicates if LENGTH field contains CRC */
# define RADIO_PCNF0_CRCINC (1 << RADIO_PCNF0_CRCINC_SHIFT)
# define RADIO_PCNF0_TERMLEN_SHIFT (29) /* Bits 29-30: Length of TERM field in Long Range operation */
# define RADIO_PCNF0_TERMLEN_MASK (0x3 << RADIO_PCNF0_TERMLEN_SHIFT)
# define RADIO_PCNF0_TERMLEN_MAX (0x3)
```
##########
File path: arch/arm/src/nrf52/hardware/nrf52_radio.h
##########
@@ -454,20 +448,27 @@
#define RADIO_PCNF0_S1LEN_MASK (0xf << RADIO_PCNF0_S1LEN_SHIFT)
#define RADIO_PCNF0_S1LEN_MAX (0xf)
#define RADIO_PCNF0_S1INCL (1 << 20) /* Bit 20: Include or exclude S1 field in RAM */
+
+#ifdef CONFIG_ARCH_CHIP_NRF52840
#define RADIO_PCNF0_CILEN_SHIFT (22) /* Bits 22-23: Length of code indicator - long range */
#define RADIO_PCNF0_CILEN_MASK (0x3 << RADIO_PCNF0_CILEN_SHIFT)
#define RADIO_PCNF0_CILEN_MAX (0x3)
+#endif
Review comment:
```suggestion
#ifdef CONFIG_ARCH_CHIP_NRF52840
# define RADIO_PCNF0_CILEN_SHIFT (22) /* Bits 22-23: Length of code indicator - long range */
# define RADIO_PCNF0_CILEN_MASK (0x3 << RADIO_PCNF0_CILEN_SHIFT)
# define RADIO_PCNF0_CILEN_MAX (0x3)
#endif
```
##########
File path: arch/arm/src/nrf52/hardware/nrf52_clock.h
##########
@@ -118,9 +118,11 @@
/* HFCLKSTAT Register */
#define CLOCK_HFCLKSTAT_SRC_SHIFT (0) /* Bit 0: Source of HFCLK */
+
Review comment:
The same applies to several places below.
##########
File path: arch/arm/src/nrf52/hardware/nrf52_clock.h
##########
@@ -118,9 +118,11 @@
/* HFCLKSTAT Register */
#define CLOCK_HFCLKSTAT_SRC_SHIFT (0) /* Bit 0: Source of HFCLK */
+
Review comment:
What is the purpose of adding these blank lines? The current convention is to keep the definitions of bits in the register as one block of code. We should be consistent in this.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org