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/08/04 17:28:23 UTC
[GitHub] [incubator-nuttx] PetervdPerk opened a new pull request, #6788: LPC17xx_40xx PWM multichannel support
PetervdPerk opened a new pull request, #6788:
URL: https://github.com/apache/incubator-nuttx/pull/6788
## Summary
Adds support for multichannel in the LPC17xx_40xx PWM
Furthermore some small changes
- USB no softconnect support
- SocketCAN Kconfig not fully implemented for #6758
- Small typos in kconfig and trace output
## Impact
Minimal only LPC17xx_40xx affected
## Testing
Tested pwm command on custom LPC1768 board.
--
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
[GitHub] [incubator-nuttx] xiaoxiang781216 merged pull request #6788: LPC17xx_40xx PWM multichannel support
Posted by GitBox <gi...@apache.org>.
xiaoxiang781216 merged PR #6788:
URL: https://github.com/apache/incubator-nuttx/pull/6788
--
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
[GitHub] [incubator-nuttx] pkarashchenko commented on a diff in pull request #6788: LPC17xx_40xx PWM multichannel support
Posted by GitBox <gi...@apache.org>.
pkarashchenko commented on code in PR #6788:
URL: https://github.com/apache/incubator-nuttx/pull/6788#discussion_r938494873
##########
arch/arm/src/lpc17xx_40xx/lpc17_40_pwm.c:
##########
@@ -96,16 +96,22 @@
/* This structure represents the state of one PWM timer */
+struct lpc17_40_pwmchan_s
+{
+ uint8_t channel; /* Timer output channel */
+ uint32_t pincfg; /* Output pin configuration */
+};
+
struct lpc17_40_pwmtimer_s
{
- const struct pwm_ops_s *ops; /* PWM operations */
- uint8_t timid; /* Timer ID {0,...,7} */
- uint8_t channel; /* Timer output channel: {1,..4} */
- uint8_t timtype; /* See the TIMTYPE_* definitions */
- uint32_t base; /* The base address of the timer */
- uint32_t pincfg; /* Output pin configuration */
- uint32_t pclk; /* The frequency of the peripheral clock
- * that drives the timer module. */
+ const struct pwm_ops_s *ops; /* PWM operations */
+ struct lpc17_40_pwmchan_s *channels; /* Channels configuration */
+ uint8_t timid; /* Timer ID {0,...,7} */
+ uint8_t chan_num; /* Number of configured channels */
+ uint8_t timtype; /* See the TIMTYPE_* definitions */
+ uint32_t base; /* The base address of the timer */
+ uint32_t pclk; /* The frequency of the peripheral clock
+ * that drives the timer module. */
Review Comment:
```suggestion
uint32_t pclk; /* The frequency of the peripheral clock
* that drives the timer module. */
```
##########
arch/arm/src/lpc17xx_40xx/lpc17_40_pwm.c:
##########
@@ -279,19 +327,135 @@ static int pwm_timer(struct lpc17_40_pwmtimer_s *priv,
const struct pwm_info_s *info)
{
irqstate_t flags;
+ uint32_t i;
+ uint32_t ret = OK;
+ uint32_t lerval = LER0_EN;
+ uint32_t pcrval = 0;
+ uint32_t mr0_freq;
flags = enter_critical_section();
- putreg32(info->frequency, LPC17_40_PWM1_MR0); /* Set PWMMR0 = number of counts */
+ mr0_freq = 1.f / info->frequency * LPC17_40_PWM_CLOCK / 10;
+
+ putreg32(mr0_freq, LPC17_40_PWM1_MR0); /* Set PWMMR0 = number of counts */
+
+#ifndef CONFIG_PWM_MULTICHAN
putreg32(info->duty, LPC17_40_PWM1_MR1); /* Set PWM cycle */
+#else
+ for (i = 0; i < CONFIG_PWM_NCHANNELS; i++)
+ {
+ switch (priv->channels[i].channel)
+ {
+#ifdef CONFIG_LPC17_40_PWM1_CHANNEL1
+ case 1:
+ {
Review Comment:
I think extra `{}` can be removed for case blocks here
##########
arch/arm/src/lpc17xx_40xx/lpc17_40_pwm.c:
##########
@@ -279,19 +327,135 @@ static int pwm_timer(struct lpc17_40_pwmtimer_s *priv,
const struct pwm_info_s *info)
{
irqstate_t flags;
+ uint32_t i;
+ uint32_t ret = OK;
+ uint32_t lerval = LER0_EN;
+ uint32_t pcrval = 0;
+ uint32_t mr0_freq;
flags = enter_critical_section();
- putreg32(info->frequency, LPC17_40_PWM1_MR0); /* Set PWMMR0 = number of counts */
+ mr0_freq = 1.f / info->frequency * LPC17_40_PWM_CLOCK / 10;
+
+ putreg32(mr0_freq, LPC17_40_PWM1_MR0); /* Set PWMMR0 = number of counts */
+
+#ifndef CONFIG_PWM_MULTICHAN
putreg32(info->duty, LPC17_40_PWM1_MR1); /* Set PWM cycle */
+#else
+ for (i = 0; i < CONFIG_PWM_NCHANNELS; i++)
+ {
+ switch (priv->channels[i].channel)
+ {
+#ifdef CONFIG_LPC17_40_PWM1_CHANNEL1
+ case 1:
+ {
+ putreg32(b16toi((uint64_t)info->channels[i].duty * mr0_freq
+ + b16HALF),
+ LPC17_40_PWM1_MR1); /* Set PWM cycle */
+ break;
+ }
+#endif
+
+#ifdef CONFIG_LPC17_40_PWM1_CHANNEL2
+ case 2:
+ {
+ putreg32(b16toi((uint64_t)info->channels[i].duty * mr0_freq
+ + b16HALF),
+ LPC17_40_PWM1_MR2); /* Set PWM cycle */
+ break;
+ }
+#endif
+
+#ifdef CONFIG_LPC17_40_PWM1_CHANNEL3
+ case 3:
+ {
+ putreg32(b16toi((uint64_t)info->channels[i].duty * mr0_freq
+ + b16HALF),
+ LPC17_40_PWM1_MR3); /* Set PWM cycle */
+ break;
+ }
+#endif
+
+#ifdef CONFIG_LPC17_40_PWM1_CHANNEL4
+ case 4:
+ {
+ putreg32(b16toi((uint64_t)info->channels[i].duty * mr0_freq
+ + b16HALF),
+ LPC17_40_PWM1_MR4); /* Set PWM cycle */
+ break;
+ }
+#endif
+
+#ifdef CONFIG_LPC17_40_PWM1_CHANNEL5
+ case 5:
+ {
+ putreg32(b16toi((uint64_t)info->channels[i].duty * mr0_freq
+ + b16HALF),
+ LPC17_40_PWM1_MR5); /* Set PWM cycle */
+ break;
+ }
+#endif
+
+#ifdef CONFIG_LPC17_40_PWM1_CHANNEL6
+ case 6:
+ {
+ putreg32(b16toi((uint64_t)info->channels[i].duty * mr0_freq
Review Comment:
Maybe better to use `ub16_t` or `ub32_t` instead of `uint64_t`?
##########
arch/arm/src/lpc17xx_40xx/lpc17_40_pwm.c:
##########
@@ -428,14 +593,17 @@ static int pwm_setup(struct pwm_lowerhalf_s *dev)
/* Select clock for the pwm peripheral */
regval = getreg32(LPC17_40_SYSCON_PCLKSEL0);
- regval &= ~(0x3 << 12); /* PCLK_MC peripheral clk = CCLK = 12.5 MHz */
- regval |= (0x1 << 12); /* PCLK_MC peripheral clk = CCLK = 12.5 MHz */
+ regval &= ~(0x3 << 12); /* PCLK_MC peripheral clk = CCLK = LPC17_40_CCLK */
+ regval |= (0x1 << 12); /* PCLK_MC peripheral clk = CCLK = LPC17_40_CCLK */
Review Comment:
```suggestion
regval &= ~SYSCON_PCLKSEL0_PWM1_MASK; /* PCLK_MC peripheral clk = CCLK = LPC17_40_CCLK */
regval |= (SYSCON_PCLKSEL_CCLK << SYSCON_PCLKSEL0_PWM1_SHIFT); /* PCLK_MC peripheral clk = CCLK = LPC17_40_CCLK */
```
##########
arch/arm/src/lpc17xx_40xx/lpc17_40_pwm.h:
##########
@@ -33,6 +33,57 @@
* Pre-processor Definitions
****************************************************************************/
+/* PLL0CLK = CCLK * CCLK divider */
+
+#define LPC17_40_PWM_CLOCK LPC17_40_CCLK * BOARD_CCLKCFG_DIVIDER
Review Comment:
```suggestion
#define LPC17_40_PWM_CLOCK (LPC17_40_CCLK * BOARD_CCLKCFG_DIVIDER)
```
##########
arch/arm/src/lpc17xx_40xx/lpc17_40_pwm.c:
##########
@@ -279,19 +327,135 @@ static int pwm_timer(struct lpc17_40_pwmtimer_s *priv,
const struct pwm_info_s *info)
{
irqstate_t flags;
+ uint32_t i;
+ uint32_t ret = OK;
Review Comment:
```suggestion
int ret = OK;
```
##########
arch/arm/src/lpc17xx_40xx/Kconfig:
##########
@@ -172,6 +172,9 @@ config ARCH_FAMILY_LPC408X
config LPC17_40_HAVE_SPIFI
bool
+config LPC17_40_CAN
+ bool
Review Comment:
```suggestion
bool
```
##########
arch/arm/src/lpc17xx_40xx/lpc17_40_pwm.c:
##########
@@ -465,11 +633,25 @@ static int pwm_shutdown(struct pwm_lowerhalf_s *dev)
{
struct lpc17_40_pwmtimer_s *priv =
(struct lpc17_40_pwmtimer_s *)dev;
- uint32_t pincfg;
+ uint32_t regval;
+ uint32_t i;
+
+ /* Configure the output pin to be output and low */
+
+ for (i = 0; i < priv->chan_num; i++)
+ {
+ regval = priv->channels[i].pincfg;
+ regval &= (GPIO_PORT_MASK | GPIO_PIN_MASK);
+ regval |= (GPIO_OUTPUT | GPIO_VALUE_ZERO);
+
+ lpc17_40_configgpio(regval);
+ }
- pwminfo("TIM%d pincfg: %08x\n", priv->timid, priv->pincfg);
+ /* Power off the pwm peripheral */
- /* Make sure that the output has been stopped */
+ regval = getreg32(LPC17_40_SYSCON_PCONP);
+ regval &= ~(SYSCON_PCONP_PCPWM1);
Review Comment:
```suggestion
regval &= ~SYSCON_PCONP_PCPWM1;
```
##########
arch/arm/src/lpc17xx_40xx/lpc17_40_pwm.h:
##########
@@ -33,6 +33,57 @@
* Pre-processor Definitions
****************************************************************************/
+/* PLL0CLK = CCLK * CCLK divider */
+
+#define LPC17_40_PWM_CLOCK LPC17_40_CCLK * BOARD_CCLKCFG_DIVIDER
+
+#ifdef CONFIG_LPC17_40_PWM1_CHANNEL1
+#define LPC17_40_PWM1_CHANNEL1 1
+#else
+#define LPC17_40_PWM1_CHANNEL1 0
+#endif
+
+#ifdef CONFIG_LPC17_40_PWM1_CHANNEL2
+#define LPC17_40_PWM1_CHANNEL2 1
+#else
+#define LPC17_40_PWM1_CHANNEL2 0
+#endif
+
+#ifdef CONFIG_LPC17_40_PWM1_CHANNEL3
+#define LPC17_40_PWM1_CHANNEL3 1
+#else
+#define LPC17_40_PWM1_CHANNEL3 0
+#endif
+
+#ifdef CONFIG_LPC17_40_PWM1_CHANNEL4
+#define LPC17_40_PWM1_CHANNEL4 1
+#else
+#define LPC17_40_PWM1_CHANNEL4 0
+#endif
+
+#ifdef CONFIG_LPC17_40_PWM1_CHANNEL5
+#define LPC17_40_PWM1_CHANNEL5 1
+#else
+#define LPC17_40_PWM1_CHANNEL5 0
+#endif
+
+#ifdef CONFIG_LPC17_40_PWM1_CHANNEL6
+#define LPC17_40_PWM1_CHANNEL6 1
+#else
+#define LPC17_40_PWM1_CHANNEL6 0
+#endif
+
+#define LPC17_40_PWM1_NCHANNELS (LPC17_40_PWM1_CHANNEL1 + \
+ LPC17_40_PWM1_CHANNEL2 + \
+ LPC17_40_PWM1_CHANNEL3 + \
+ LPC17_40_PWM1_CHANNEL4 + \
+ LPC17_40_PWM1_CHANNEL5 + \
+ LPC17_40_PWM1_CHANNEL6)
+
+#if CONFIG_PWM_NCHANNELS > LPC17_40_PWM1_NCHANNELS
+# error "PWM subystem has more channels then physical channels enabled"
Review Comment:
```suggestion
# error "PWM subystem has more channels then physical channels enabled"
```
##########
arch/arm/src/lpc17xx_40xx/lpc17_40_pwm.h:
##########
@@ -33,6 +33,57 @@
* Pre-processor Definitions
****************************************************************************/
+/* PLL0CLK = CCLK * CCLK divider */
+
+#define LPC17_40_PWM_CLOCK LPC17_40_CCLK * BOARD_CCLKCFG_DIVIDER
+
+#ifdef CONFIG_LPC17_40_PWM1_CHANNEL1
+#define LPC17_40_PWM1_CHANNEL1 1
Review Comment:
```suggestion
# define LPC17_40_PWM1_CHANNEL1 1
```
here and other similar places
--
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
[GitHub] [incubator-nuttx] PetervdPerk commented on a diff in pull request #6788: LPC17xx_40xx PWM multichannel support
Posted by GitBox <gi...@apache.org>.
PetervdPerk commented on code in PR #6788:
URL: https://github.com/apache/incubator-nuttx/pull/6788#discussion_r939049907
##########
arch/arm/src/lpc17xx_40xx/lpc17_40_pwm.c:
##########
@@ -279,19 +327,135 @@ static int pwm_timer(struct lpc17_40_pwmtimer_s *priv,
const struct pwm_info_s *info)
{
irqstate_t flags;
+ uint32_t i;
+ uint32_t ret = OK;
+ uint32_t lerval = LER0_EN;
+ uint32_t pcrval = 0;
+ uint32_t mr0_freq;
flags = enter_critical_section();
- putreg32(info->frequency, LPC17_40_PWM1_MR0); /* Set PWMMR0 = number of counts */
+ mr0_freq = 1.f / info->frequency * LPC17_40_PWM_CLOCK / 10;
+
+ putreg32(mr0_freq, LPC17_40_PWM1_MR0); /* Set PWMMR0 = number of counts */
+
+#ifndef CONFIG_PWM_MULTICHAN
putreg32(info->duty, LPC17_40_PWM1_MR1); /* Set PWM cycle */
+#else
+ for (i = 0; i < CONFIG_PWM_NCHANNELS; i++)
+ {
+ switch (priv->channels[i].channel)
+ {
+#ifdef CONFIG_LPC17_40_PWM1_CHANNEL1
+ case 1:
+ {
+ putreg32(b16toi((uint64_t)info->channels[i].duty * mr0_freq
+ + b16HALF),
+ LPC17_40_PWM1_MR1); /* Set PWM cycle */
+ break;
+ }
+#endif
+
+#ifdef CONFIG_LPC17_40_PWM1_CHANNEL2
+ case 2:
+ {
+ putreg32(b16toi((uint64_t)info->channels[i].duty * mr0_freq
+ + b16HALF),
+ LPC17_40_PWM1_MR2); /* Set PWM cycle */
+ break;
+ }
+#endif
+
+#ifdef CONFIG_LPC17_40_PWM1_CHANNEL3
+ case 3:
+ {
+ putreg32(b16toi((uint64_t)info->channels[i].duty * mr0_freq
+ + b16HALF),
+ LPC17_40_PWM1_MR3); /* Set PWM cycle */
+ break;
+ }
+#endif
+
+#ifdef CONFIG_LPC17_40_PWM1_CHANNEL4
+ case 4:
+ {
+ putreg32(b16toi((uint64_t)info->channels[i].duty * mr0_freq
+ + b16HALF),
+ LPC17_40_PWM1_MR4); /* Set PWM cycle */
+ break;
+ }
+#endif
+
+#ifdef CONFIG_LPC17_40_PWM1_CHANNEL5
+ case 5:
+ {
+ putreg32(b16toi((uint64_t)info->channels[i].duty * mr0_freq
+ + b16HALF),
+ LPC17_40_PWM1_MR5); /* Set PWM cycle */
+ break;
+ }
+#endif
+
+#ifdef CONFIG_LPC17_40_PWM1_CHANNEL6
+ case 6:
+ {
+ putreg32(b16toi((uint64_t)info->channels[i].duty * mr0_freq
Review Comment:
Thanks for the suggestion found out that `ub16mulub16` did exactly what I needed.
--
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