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 2022/11/19 15:15:12 UTC
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #7614: SAMA5D2 pio errors
pkarashchenko commented on code in PR #7614:
URL: https://github.com/apache/incubator-nuttx/pull/7614#discussion_r1027102529
##########
arch/arm/src/sama5/Kconfig:
##########
@@ -3244,13 +3244,15 @@ config SAMA5_ADC_SEQUENCER
config SAMA5_ADC_ANARCH
bool "Analog changes"
default n
+ depends on ARCH_CHIP_SAMA5D3 || ARCH_CHIP_SAMA5D2
---help---
This option allows you to select different gain, offset, and single
vs. differential modes for each channel.
if SAMA5_ADC_ANARCH
menu "Channel gain"
+depends on ARCH_CHIP_SAMA5D3
Review Comment:
```suggestion
depends on ARCH_CHIP_SAMA5D3
```
##########
arch/arm/src/sama5/Kconfig:
##########
@@ -3627,12 +3627,45 @@ config SAMA5_ADC_TIOATRIG
A-to-D Conversion is initiated the A output from one of
Timer/Counter 0 channels.
+config SAMA5_ADC_PWMTRIG
+ bool "PWM Event trigger"
+ depends on SAMA5_PWM
+ ---help---
+ A-to-D Conversion is initiated from the PWM event lines
+
+config SAMA5_ADC_RTCOUT
+ bool "RTC Out trigger"
+ depends on SAMA5_RTC
+ depends on ARCH_CHIP_SAMA5D2
+ ---help---
+ A-to-D Conversion is initiated from the RTC output
+
endchoice # Trigger mode
+if SAMA5_ADC_PWMTRIG
+
+choice
+ prompt "PWM Event Line Selection"
+ default SAMA5_ADC_PWMTRIG_LINE0
+
+config SAMA5_ADC_PWM_TRIG_LINE0
+ bool "PWM event Line 0"
+ ---help---
+ Trigger A-to-D conversion on PWM event line 0
Review Comment:
TABs
##########
arch/arm/src/sama5/sam_tc.c:
##########
@@ -1460,7 +1461,7 @@ int sam_tc_divisor(uint32_t frequency, uint32_t *div, uint32_t *tcclks)
uint32_t ftcin = sam_tc_infreq();
int ndx = 0;
- tmrinfo("frequency=%d\n", frequency);
+ tmrinfo("frequency=%ld\n", frequency);
Review Comment:
```suggestion
tmrinfo("frequency=%" PRIu32 "\n", frequency);
```
##########
arch/arm/src/sama5/sama5d2x_pio.c:
##########
@@ -262,6 +262,32 @@ static uint32_t sam_configcommon(pio_pinset_t cfgset)
break;
}
+ /* Select Input Event selection.
+ * NOTE: Only applies to input pins
+ */
+
+ switch (cfgset & PIO_INT_MASK)
+ {
+ default:
+ case PIO_INT_NONE:
+ break;
+ case PIO_INT_FALLING:
+ regval |= PIO_CFGR_EVTSEL_FALLING;
+ break;
Review Comment:
```suggestion
break;
```
##########
arch/arm/src/sama5/sama5d2x_pio.c:
##########
@@ -262,6 +262,32 @@ static uint32_t sam_configcommon(pio_pinset_t cfgset)
break;
}
+ /* Select Input Event selection.
+ * NOTE: Only applies to input pins
+ */
+
+ switch (cfgset & PIO_INT_MASK)
+ {
+ default:
+ case PIO_INT_NONE:
+ break;
+ case PIO_INT_FALLING:
+ regval |= PIO_CFGR_EVTSEL_FALLING;
+ break;
+ case PIO_INT_RISING:
+ regval |= PIO_CFGR_EVTSEL_RISING;
+ break;
+ case PIO_INT_BOTHEDGES:
+ regval |= PIO_CFGR_EVTSEL_BOTH;
+ break;
+ case PIO_INT_LOWLEVEL:
+ regval |= PIO_CFGR_EVTSEL_LOW;
+ break;
Review Comment:
```suggestion
break;
```
##########
arch/arm/src/sama5/Kconfig:
##########
@@ -3487,6 +3490,7 @@ config SAMA5_ADC_OFFSET11
endmenu # Channel offsets
menu "Channel differential mode"
+depends on ARCH_CHIP_SAMA5D3 || ARCH_CHIP_SAMA5D2
Review Comment:
```suggestion
depends on ARCH_CHIP_SAMA5D3 || ARCH_CHIP_SAMA5D2
```
##########
arch/arm/src/sama5/Kconfig:
##########
@@ -3627,12 +3627,45 @@ config SAMA5_ADC_TIOATRIG
A-to-D Conversion is initiated the A output from one of
Timer/Counter 0 channels.
+config SAMA5_ADC_PWMTRIG
+ bool "PWM Event trigger"
+ depends on SAMA5_PWM
+ ---help---
+ A-to-D Conversion is initiated from the PWM event lines
+
+config SAMA5_ADC_RTCOUT
+ bool "RTC Out trigger"
+ depends on SAMA5_RTC
+ depends on ARCH_CHIP_SAMA5D2
+ ---help---
+ A-to-D Conversion is initiated from the RTC output
+
endchoice # Trigger mode
+if SAMA5_ADC_PWMTRIG
+
+choice
+ prompt "PWM Event Line Selection"
+ default SAMA5_ADC_PWMTRIG_LINE0
Review Comment:
TABs
##########
arch/arm/src/sama5/Kconfig:
##########
@@ -3573,11 +3577,11 @@ config SAMA5_ADC_DIFFMODE11
Selects differential (vs. single-ended mode) for ADC channel 11
endmenu # Differential mode
-endif # SAMA5_ADC_ANARCH
-if !SAMA5_ADC_ANARCH
+endif # SAMA5_ADC_ANARCH
config SAMA5_ADC_GAIN
+depends on ARCH_CHIP_SAMA5D3
Review Comment:
```suggestion
depends on ARCH_CHIP_SAMA5D3
```
##########
arch/arm/src/sama5/Kconfig:
##########
@@ -3244,13 +3244,15 @@ config SAMA5_ADC_SEQUENCER
config SAMA5_ADC_ANARCH
bool "Analog changes"
default n
+ depends on ARCH_CHIP_SAMA5D3 || ARCH_CHIP_SAMA5D2
Review Comment:
TABs
##########
arch/arm/src/sama5/Kconfig:
##########
@@ -3387,6 +3389,7 @@ config SAMA5_ADC_GAIN11
endmenu # Channel gain
menu "Channel offsets"
+depends on ARCH_CHIP_SAMA5D3
Review Comment:
```suggestion
depends on ARCH_CHIP_SAMA5D3
```
##########
arch/arm/src/sama5/Kconfig:
##########
@@ -3712,6 +3746,17 @@ config SAMA5_ADC_TIOA_BOTH
endchoice # ADTRG edge
endif # SAMA5_ADC_TIOATRIG
+
+if SAMA5_ADC_PERIODIC_TRIG
+
+config SAMA5_ADC_TRIGGER_PERIOD
+ int "ADC Periodic Trigger Rate, useconds"
+ default 50000
+ ---help---
+ This setting determines the periodic sample trigger rate in useconds.
Review Comment:
TABs
##########
arch/arm/src/sama5/Kconfig:
##########
@@ -3627,12 +3627,45 @@ config SAMA5_ADC_TIOATRIG
A-to-D Conversion is initiated the A output from one of
Timer/Counter 0 channels.
+config SAMA5_ADC_PWMTRIG
+ bool "PWM Event trigger"
+ depends on SAMA5_PWM
+ ---help---
+ A-to-D Conversion is initiated from the PWM event lines
+
+config SAMA5_ADC_RTCOUT
+ bool "RTC Out trigger"
+ depends on SAMA5_RTC
+ depends on ARCH_CHIP_SAMA5D2
+ ---help---
+ A-to-D Conversion is initiated from the RTC output
+
endchoice # Trigger mode
+if SAMA5_ADC_PWMTRIG
+
+choice
+ prompt "PWM Event Line Selection"
+ default SAMA5_ADC_PWMTRIG_LINE0
+
+config SAMA5_ADC_PWM_TRIG_LINE0
+ bool "PWM event Line 0"
+ ---help---
+ Trigger A-to-D conversion on PWM event line 0
+
+config SAMA5_ADC_PWM_TRIG_LINE1
+ bool "PWM event Line 1"
+ ---help---
+ Trigger A-to-D conversion on PWM event line 1
Review Comment:
TABs
##########
arch/arm/src/sama5/Kconfig:
##########
@@ -3627,12 +3627,45 @@ config SAMA5_ADC_TIOATRIG
A-to-D Conversion is initiated the A output from one of
Timer/Counter 0 channels.
+config SAMA5_ADC_PWMTRIG
+ bool "PWM Event trigger"
+ depends on SAMA5_PWM
+ ---help---
+ A-to-D Conversion is initiated from the PWM event lines
Review Comment:
TABs
##########
arch/arm/src/sama5/sam_adc.c:
##########
@@ -1728,7 +1890,8 @@ static void sam_adc_gain(struct sam_adc_s *priv)
/* Set GAIN0 only. GAIN0 will be used for all channels. */
sam_adc_putreg(priv, SAM_ADC_CGR, ADC_CGR_GAIN0(CONFIG_SAMA5_ADC_GAIN));
-#endif
+#endif /* CONFIG_SAMA5_ADC_ANARCH */
Review Comment:
```suggestion
# endif /* CONFIG_SAMA5_ADC_ANARCH */
```
##########
arch/arm/src/sama5/Kconfig:
##########
@@ -3627,12 +3627,45 @@ config SAMA5_ADC_TIOATRIG
A-to-D Conversion is initiated the A output from one of
Timer/Counter 0 channels.
+config SAMA5_ADC_PWMTRIG
+ bool "PWM Event trigger"
+ depends on SAMA5_PWM
+ ---help---
+ A-to-D Conversion is initiated from the PWM event lines
+
+config SAMA5_ADC_RTCOUT
+ bool "RTC Out trigger"
+ depends on SAMA5_RTC
+ depends on ARCH_CHIP_SAMA5D2
+ ---help---
+ A-to-D Conversion is initiated from the RTC output
Review Comment:
TABs
##########
arch/arm/src/sama5/sam_adc.c:
##########
@@ -2050,9 +2213,17 @@ struct adc_dev_s *sam_adc_initialize(void)
/* Initialize the public ADC device data structure */
#ifdef SAMA5_ADC_HAVE_CHANNELS
+ g_adcdev.ad_ops = &g_adcops;
priv->dev = &g_adcdev;
#endif
+ g_adcdev.ad_priv = priv;
+
+ /* Initialize the private ADC device data structure */
+
+ nxmutex_init(&priv->lock);
+ priv->cb = NULL;
Review Comment:
```suggestion
priv->cb = NULL;
```
##########
arch/arm/src/sama5/sam_tc.c:
##########
@@ -599,8 +599,8 @@ static inline uint32_t sam_tc_getreg(struct sam_chan_s *chan,
*
****************************************************************************/
-static inline void sam_tc_putreg(struct sam_chan_s *chan, uint32_t regval,
- unsigned int offset)
+static inline void sam_tc_putreg(struct sam_chan_s *chan,
+ unsigned int offset, uint32_t regval)
Review Comment:
```suggestion
unsigned int offset, uint32_t regval)
```
##########
arch/arm/src/sama5/sam_adc.c:
##########
@@ -2050,9 +2213,17 @@ struct adc_dev_s *sam_adc_initialize(void)
/* Initialize the public ADC device data structure */
#ifdef SAMA5_ADC_HAVE_CHANNELS
+ g_adcdev.ad_ops = &g_adcops;
Review Comment:
```suggestion
g_adcdev.ad_ops = &g_adcops;
```
##########
arch/arm/src/sama5/sam_tsd.c:
##########
@@ -137,6 +137,18 @@
# define MAX(a,b) (((a) > (b)) ? (a) : (b))
#endif
+#ifndef BOARD_TSSCTIM
+# define BOARD_TSSCTIM 0
Review Comment:
```suggestion
# define BOARD_TSSCTIM 0
```
##########
arch/arm/src/sama5/sam_tsd.c:
##########
@@ -245,12 +257,7 @@ static const struct file_operations g_tsdops =
/* The driver state structure is pre-allocated. */
-static struct sam_tsd_s g_tsd =
-{
- .threshx = INVALID_THRESHOLD,
- .threshy = INVALID_THRESHOLD,
- .waitsem = SEM_INITIALIZER(0),
-};
+static struct sam_tsd_s g_tsd;
Review Comment:
Why static init is removed?
##########
arch/arm/src/sama5/sam_tsd.c:
##########
@@ -1655,7 +1735,10 @@ int sam_tsd_register(struct sam_adc_s *adc, int minor)
/* Initialize the touchscreen device driver instance */
- priv->adc = adc; /* Save the ADC device handle */
+ priv->adc = adc; /* Save the ADC device handle */
+ priv->threshx = INVALID_THRESHOLD; /* Initialize thresholding logic */
+ priv->threshy = INVALID_THRESHOLD; /* Initialize thresholding logic */
+ nxsem_init(&priv->waitsem, 0, 0);
Review Comment:
Why those fields can't be initialized statically?
##########
arch/arm/src/sama5/sam_tsd.c:
##########
@@ -1318,7 +1378,11 @@ static void sam_tsd_tracking(struct sam_tsd_s *priv, uint32_t time)
tracktim--;
}
}
-
+#elif defined (ATSAMA5D3)
Review Comment:
```suggestion
#elif defined(ATSAMA5D3)
```
##########
arch/arm/src/sama5/sam_tsd.c:
##########
@@ -460,9 +468,11 @@ static void sam_tsd_setaverage(struct sam_tsd_s *priv, uint32_t tsav)
{
/* Set TSFREQ = TSAV */
- regval &= ~ADC_TSMR_TSFREQ_MASK;
- regval |= ADC_TSMR_TSFREQ(minfreq);
+ tsfreq = minfreq;
}
+
+ regval &= ~ADC_TSMR_TSFREQ_MASK;
+ regval |= ADC_TSMR_TSFREQ(minfreq);
Review Comment:
```suggestion
regval |= ADC_TSMR_TSFREQ(minfreq);
```
##########
arch/arm/src/sama5/sam_tsd.c:
##########
@@ -639,6 +684,7 @@ static void sam_tsd_bottomhalf(void *arg)
yraw, yscale);
goto ignored;
}
+#endif
Review Comment:
```suggestion
```
##########
arch/arm/src/sama5/sama5d2x_pio.c:
##########
@@ -262,6 +262,32 @@ static uint32_t sam_configcommon(pio_pinset_t cfgset)
break;
}
+ /* Select Input Event selection.
+ * NOTE: Only applies to input pins
+ */
+
+ switch (cfgset & PIO_INT_MASK)
+ {
+ default:
+ case PIO_INT_NONE:
+ break;
+ case PIO_INT_FALLING:
+ regval |= PIO_CFGR_EVTSEL_FALLING;
+ break;
+ case PIO_INT_RISING:
+ regval |= PIO_CFGR_EVTSEL_RISING;
+ break;
Review Comment:
```suggestion
break;
```
##########
arch/arm/src/sama5/sam_tsd.c:
##########
@@ -630,7 +675,7 @@ static void sam_tsd_bottomhalf(void *arg)
pressr = sam_adc_getreg(priv->adc, SAM_ADC_PRESSR);
#endif
/* Discard any bad readings. This check may not be necessary. */
-
+#if 1
Review Comment:
```suggestion
```
##########
arch/arm/src/sama5/sama5d2x_pio.c:
##########
@@ -262,6 +262,32 @@ static uint32_t sam_configcommon(pio_pinset_t cfgset)
break;
}
+ /* Select Input Event selection.
+ * NOTE: Only applies to input pins
+ */
+
+ switch (cfgset & PIO_INT_MASK)
+ {
+ default:
+ case PIO_INT_NONE:
+ break;
Review Comment:
```suggestion
break;
```
##########
arch/arm/src/sama5/sama5d2x_pio.c:
##########
@@ -262,6 +262,32 @@ static uint32_t sam_configcommon(pio_pinset_t cfgset)
break;
}
+ /* Select Input Event selection.
+ * NOTE: Only applies to input pins
+ */
+
+ switch (cfgset & PIO_INT_MASK)
+ {
+ default:
+ case PIO_INT_NONE:
+ break;
+ case PIO_INT_FALLING:
+ regval |= PIO_CFGR_EVTSEL_FALLING;
+ break;
+ case PIO_INT_RISING:
+ regval |= PIO_CFGR_EVTSEL_RISING;
+ break;
+ case PIO_INT_BOTHEDGES:
+ regval |= PIO_CFGR_EVTSEL_BOTH;
+ break;
Review Comment:
```suggestion
break;
```
##########
arch/arm/src/sama5/hardware/_sama5d2x_memorymap.h:
##########
@@ -517,9 +520,10 @@
#define SAM_SFRBU_VBASE (SAM_PERIPHC_VSECTION+SAM_SFRBU_OFFSET)
#define SAM_CHIPID_VBASE (SAM_PERIPHC_VSECTION+SAM_CHIPID_OFFSET)
-#define SAM_PIOA_VBASE (SAM_PERIPHA_VSECTION+SAM_PIO_OFFSET)
-#define SAM_PIOB_VBASE (SAM_PERIPHB_VSECTION+SAM_PIO_OFFSET)
-#define SAM_PIOC_VBASE (SAM_PERIPHC_VSECTION+SAM_PIO_OFFSET)
+#define SAM_PIOA_VBASE (SAM_PIO_VBASE)
+#define SAM_PIOB_VBASE (SAM_PIO_VBASE+SAM_PIOB_OFFSET)
+#define SAM_PIOC_VBASE (SAM_PIO_VBASE+SAM_PIOC_OFFSET)
+#define SAM_PIOD_VBASE (SAM_PIO_VBASE+SAM_PIOD_OFFSET)
Review Comment:
Maybe better to add `SAM_PERIPHB_VSECTION`, `SAM_PERIPHC_VSECTION` and `SAM_PERIPHD_VSECTION` just from style perspective? And define:
```
# define SAM_PIO_OFFSET 0x00038000 /* 0x00038000-0x0003bfff: PIOA-D */
# define SAM_PIOA_OFFSET 0x00038000 /* PIOA */
# define SAM_PIOB_OFFSET 0x00038040 /* PIOB */
# define SAM_PIOC_OFFSET 0x00038080 /* PIOC */
# define SAM_PIOD_OFFSET 0x000380c0 /* PIOD */
```
?
##########
arch/arm/src/sama5/sama5d2x_pio.c:
##########
@@ -262,6 +262,32 @@ static uint32_t sam_configcommon(pio_pinset_t cfgset)
break;
}
+ /* Select Input Event selection.
+ * NOTE: Only applies to input pins
+ */
+
+ switch (cfgset & PIO_INT_MASK)
+ {
+ default:
+ case PIO_INT_NONE:
+ break;
+ case PIO_INT_FALLING:
+ regval |= PIO_CFGR_EVTSEL_FALLING;
+ break;
+ case PIO_INT_RISING:
+ regval |= PIO_CFGR_EVTSEL_RISING;
+ break;
+ case PIO_INT_BOTHEDGES:
+ regval |= PIO_CFGR_EVTSEL_BOTH;
+ break;
+ case PIO_INT_LOWLEVEL:
+ regval |= PIO_CFGR_EVTSEL_LOW;
+ break;
+ case PIO_INT_HIGHLEVEL:
+ regval |= PIO_CFGR_EVTSEL_HIGH;
+ break;
Review Comment:
```suggestion
break;
```
--
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.
To unsubscribe, e-mail: commits-unsubscribe@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org