[GitHub] mlaz commented on a change in pull request #1169: nrf52_pwm: catch NRFX_PWM_EVT_STOPPED events

[GitBox <gi...@apache.org>]

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:
users@infra.apache.org


With regards,
Apache Git Services