michallenc commented on code in PR #6298:
URL: https://github.com/apache/incubator-nuttx/pull/6298#discussion_r876885764
##########
arch/arm/src/samv7/sam_pwm.c:
##########
@@ -493,71 +449,39 @@ static int pwm_start(struct pwm_lowerhalf_s *dev,
const struct pwm_info_s *info)
{
struct sam_pwm_s *priv = (struct sam_pwm_s *)dev;
- int ret = OK;
- /* Change frequency only if it is needed */
-
- if (info->frequency != priv->frequency)
- {
+#ifdef CONFIG_PWM_MULTICHAN
for (int i = 0; i < PWM_NCHANNELS; i++)
{
-#ifdef CONFIG_PWM_MULTICHAN
+ int8_t index = info->channels[i].channel;
+
/* Break the loop if all following channels are not configured */
- if (info->channels[i].channel == -1)
+ if (index == -1)
{
break;
}
/* Configure the module freq only if is set to be used */
- if (info->channels[i].channel != 0)
+ if (index != 0)
{
- ret = pwm_change_freq(dev, info, i);
- }
-#else
- ret = pwm_change_freq(dev, info, i);
-#endif
- }
-
- /* Save current frequency */
-
- if (ret == OK)
- {
- priv->frequency = info->frequency;
- }
- }
+ /* Set the frequency and enable PWM output for each channel */
-#ifdef CONFIG_PWM_MULTICHAN
- for (int i = 0; ret == OK && i < PWM_NCHANNELS; i++)
- {
- /* Break the loop if all following channels are not configured */
-
- if (info->channels[i].channel == -1)
- {
- break;
- }
-
- /* Enable PWM output for each channel */
-
- if (info->channels[i].channel != 0)
- {
- ret = pwm_set_output(dev, info->channels[i].channel - 1,
- info->channels[i].duty);
+ pwm_set_freq(dev, priv->channels[index - 1].channel,
Review Comment:
Should not the mapping be vice versa?. Or the question is what kind of
numbers are we putting into `info->channels`. The number of channel (from 1 to
4 - CH0 = 1, CH1=2 etc...) or the position number (first configured channel
equals 1, second equals 2 etc.)?
My understanding is that if we have config options `CONFIG_SAMV7_PWM0_CH0`
to `CONFIG_SAMV7_PWM0_CH3` and we select `CONFIG_SAMV7_PWM0_CH2` and
`CONFIG_SAMV7_PWM0_CH3` then channels' numbers put into `info->channels` should
be 3 and 4 (we cannot use 0 so we need to index from 1 and this is third and
fourth channel in SAM's PWM). It is better than mapping first and second
channel from my point of view because user does not need to check what other
channels are configured. I wasnt able to find any standard aproach in the
documentation thought.
##########
arch/arm/src/samv7/sam_pwm.c:
##########
@@ -493,71 +449,39 @@ static int pwm_start(struct pwm_lowerhalf_s *dev,
const struct pwm_info_s *info)
{
struct sam_pwm_s *priv = (struct sam_pwm_s *)dev;
- int ret = OK;
- /* Change frequency only if it is needed */
-
- if (info->frequency != priv->frequency)
- {
+#ifdef CONFIG_PWM_MULTICHAN
for (int i = 0; i < PWM_NCHANNELS; i++)
{
-#ifdef CONFIG_PWM_MULTICHAN
+ int8_t index = info->channels[i].channel;
+
/* Break the loop if all following channels are not configured */
- if (info->channels[i].channel == -1)
+ if (index == -1)
{
break;
}
/* Configure the module freq only if is set to be used */
- if (info->channels[i].channel != 0)
+ if (index != 0)
{
- ret = pwm_change_freq(dev, info, i);
- }
-#else
- ret = pwm_change_freq(dev, info, i);
-#endif
- }
-
- /* Save current frequency */
-
- if (ret == OK)
- {
- priv->frequency = info->frequency;
- }
- }
+ /* Set the frequency and enable PWM output for each channel */
-#ifdef CONFIG_PWM_MULTICHAN
- for (int i = 0; ret == OK && i < PWM_NCHANNELS; i++)
- {
- /* Break the loop if all following channels are not configured */
-
- if (info->channels[i].channel == -1)
- {
- break;
- }
-
- /* Enable PWM output for each channel */
-
- if (info->channels[i].channel != 0)
- {
- ret = pwm_set_output(dev, info->channels[i].channel - 1,
- info->channels[i].duty);
+ pwm_set_freq(dev, priv->channels[index - 1].channel,
Review Comment:
Should not the mapping be vice versa? Or the question is what kind of
numbers are we putting into `info->channels`. The number of channel (from 1 to
4 - CH0 = 1, CH1=2 etc...) or the position number (first configured channel
equals 1, second equals 2 etc.)?
My understanding is that if we have config options `CONFIG_SAMV7_PWM0_CH0`
to `CONFIG_SAMV7_PWM0_CH3` and we select `CONFIG_SAMV7_PWM0_CH2` and
`CONFIG_SAMV7_PWM0_CH3` then channels' numbers put into `info->channels` should
be 3 and 4 (we cannot use 0 so we need to index from 1 and this is third and
fourth channel in SAM's PWM). It is better than mapping first and second
channel from my point of view because user does not need to check what other
channels are configured. I wasnt able to find any standard aproach in the
documentation thought.
--
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-unsubscr...@nuttx.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
