drivers: pwm: extend API to support events

[javlands]

• Extend the pwm API to also support pwm interrupts.
• Implement pwm interrupts for the SAM controllers

[pdgendt]

Update tests/drivers/build_all/pwm/testcase.yaml with:

diff --git a/tests/drivers/build_all/pwm/testcase.yaml b/tests/drivers/build_all/pwm/testcase.yaml
index 34156300b5d..270637d4625 100644
--- a/tests/drivers/build_all/pwm/testcase.yaml
+++ b/tests/drivers/build_all/pwm/testcase.yaml
@@ -43,6 +43,8 @@ tests:
     platform_allow:
       - sam_e70_xplained/same70q21
       - sam_v71_xult/samv71q21b
+    extra_configs:
+      - CONFIG_PWM_INTERRUPT=y
   drivers.pwm.stm32.build:
     platform_allow: disco_l475_iot1
   drivers.pwm.xec.build:

[pdgendt]

Ping to all reviewers

Should this be brought to the architecture working group meeting?

[henrikbrixandersen]

Should this be brought to the architecture working group meeting?

I believe it should be. I am not convinced this is the right approach. We typically do not just expose IRQs to the application like this.

[pdgendt]

I am not convinced this is the right approach. We typically do not just expose IRQs to the application like this.

What's the alternative? BTW, this is also very similar to the PWM capture API.

A usecase would be to use it for IR input/output, where I'd expect similar APIs. Envision a "remote control" subsystem using these drivers as carriers.

[henrikbrixandersen]

What's the alternative? BTW, this is also very similar to the PWM capture API.

I don't think it is. The PWM capture callback provides the captured values upon a complete capture. The API proposed here merely forwards an IRQ from the device to the upper layer (subsystem or application).

A usecase would be to use it for IR input/output, where I'd expect similar APIs. Envision a "remote control" subsystem using these drivers as carriers.

For that, wouldn't it be more abstract to offload the "train" of PWM cycles to be handled in the driver/device itself? E.g. "perform N PWM cycles with period X, pulse width Y with callback on completion", or even the same but with multiple encoded PWM configurations to be "played" like a stream"

With the API proposed here, the subsystem would receive a callback upon each completed PWM cycle and have to respond to that, preventing hardware offload for capable devices.

[henrikbrixandersen]

• Extend the pwm API to also support pwm interrupts.

@javlands Please provide some context to this. What is your use-case?

[pdgendt]

@javlands Please provide some context to this. What is your use-case?

I can elaborate for my colleague

The use-case is a PWM peripheral connected to an IR LED, to emit an RC (remote control) code it is typically required to encode this by alternating pulses and spaces. Tracking the number of periods is desired to have an accurate carrier representation of an RC code.
In order to not rely on timers, we want to introduce a PWM interrupt callback mechanism.

[bjarki-andreasen]

The additions in this PR are too closely coupled to the interrupt part, rather than the "event" part. User's care when an event occurs, like PERIOD or FAULT, not how this event physically originated. Stuff like enabling an interrupt, reacting to it and clearing the pending irq flags, is not relevant to the user. All a user cares about is this part:

typedef void (*pwm_event_callback_t)(const struct device *dev, uint32_t events, void *user_data);
int pwm_event_callback_set(const struct device *dev, uint32_t event_mask, pwm_event_callback_t callback, void *user_data);
int pwm_event_callback_clear(const struct device *dev, pwm_event_callback_t callback);

whether this event occurs via interrupt, or via reading a status register of an external device, triggered by an interrupt pin for example, is not relevant to the user. So the API should abstract that part away :)

[carlescufi]

Architecture WG meeting:

• The use case here is discussed in the call
• The comment by @bjarki-andreasen here raises no objections, seems like a good idea to everyone
• @carlescufi suggests a solution to the hardware acceleration issue raised by @henrikbrixandersen in this comment:
  • Start with the API in this PR (with the changes suggested by @bjarki-andreasen)
  • Later introduce a new function that allows the user to trigger a fixed set of N cycles (all with the same period and pulse):
    • int pwm_set_n_cycles(const struct device *dev, uint32_t channel, uint32_t period, uint32_t pulse, uint32_t n, pwm_flags_t flags);
  • At the same time introduce a new event in the event mask that triggers a callback at the end of the N cycles programmed

[ts4ling]

I get @henrikbrixandersen 's concerns ("preventing hardware offload") in a general view, but for my stepper use-case it doesn't matter. For my drv8849 stepper driver I'm using k_usleep() between every ramp up/down PWM-step and when I'm not ramping I just sleep longer. That's pretty ugly. I'd love to use a PWM interrupt to do some counting.

[sonarqubecloud]

Quality Gate passed

Issues
3 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[javlands]

Hi all! Thanks for the feedback, I've processed it all:

A renaming was done from "interrupt" to "event", and the gpio API was used as a reference to (re)build the API:

• The event API now consists of 1 function manage_event_callback, that is extended by 2 public functions pwm_add_event_callback() and pwm_remove_event_callback().
• A callback struct was added to easily handle multiple callbacks for multiple channels
• pwm_utils.h contains some generic implementations to cleanly handle all callbacks

[henrikbrixandersen]

A renaming was done from "interrupt" to "event", and the gpio API was used as a reference to (re)build the API:

Please update the PR title and description to match. Thanks.