mlaz commented on a change in pull request #1169: nrf52_pwm: catch
NRFX_PWM_EVT_STOPPED events
URL: https://github.com/apache/mynewt-core/pull/1169#discussion_r193605363
##########
File path: hw/drivers/pwm/pwm_nrf52/src/pwm_nrf52.c
##########
@@ -336,7 +342,7 @@ nrf52_pwm_close(struct os_dev *odev)
}
if (!instances[inst_id].playing) {
- nrfx_pwm_uninit(&instances[inst_id].drv_instance);
+ nrf52_pwm_disable(dev);
Review comment:
> Actually, you're duplicating all these checks now, they mostly exist
inside nrf52_pwm_disable. Take another look at this
There is only a single repeated check, which is `instances[inst_id].in_use`,
we can have a static fuction (i.e. `disable_instance`) for stopping this being
called from both `nrf52_pwm_disable` and `nrf52_pwm_close`.
>why does nrf52_pwm_disable not check for playing before uninit,but
nrf52_pwm_close does?
On close we don't need to stop and `uninit` the instance if it is not
playing which is what we may expect any caller for disable to do. However
`nrfx_pwm_uninit` may not need the instanced to be initialized `nrfx_pwm_stop`
does. Adding this check to `nrf52_pwm_disable` is a good idea, but we'd still
need it on `close`.
>Again, whether it should call nrf52_pwm_disable from nrf52_pwm_close I dont
have an opinion, it just seems like were sharing code, but doing things
differently in both... which seems odd
The device should be uninitialized on `close`. Do we need to stop it before?
No. Do I think we should try to do both uninitializations the same
way?(ultimately for the sake consistency/readability) Yes.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
With regards,
Apache Git Services
