michallenc commented on code in PR #6298:
URL: https://github.com/apache/incubator-nuttx/pull/6298#discussion_r876673467
##########
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:
I am not sure about this. Lets say we configure only channels 2 and 3.
`int8_t index = info->channels[i].channel;` on line 456 gives as `index = 3`
for channel 3. But then we would be out of `priv->channels[]` range with
`priv->channels[3-1]` since only two channels are configured. There is no
assurance all 4 channels will be configured.
I think the previous `info->channels[i].channel - 1` is sufficient here, all
we need to do is just decrement the channel number used from application (from
1 to 4) so it would match register's offset (0 to 3).
The same goes for` pwm_set_output(dev, priv->channels[index - 1].channel,
info->channels[i].duty);`.
I need to go now, I will be able to try the code in few hours.
--
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
