Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify PWM driver: remove CONFIG_PWM_MULTICHAN #15436

Open
1 task done
raiden00pl opened this issue Jan 6, 2025 · 3 comments
Open
1 task done

Simplify PWM driver: remove CONFIG_PWM_MULTICHAN #15436

raiden00pl opened this issue Jan 6, 2025 · 3 comments

Comments

@raiden00pl
Copy link
Member

Description

CONFIG_PWM_MULTICHAN seems to be redundant and unnecessarily complicates the PWM implementation in arch/. We can achieve the same functionality as CONFIG_PWM_MUTLICHAN=n by just setting CONFIG_PWM_NCHANNELS=1 which is the default value in Kconfig.
If a given architecture doesn't support multichanel PWM, then we can just add a simple #if CONFIG_PWM_NCHANNELS > 1 which returns a compilation error.

Verification

  • I have verified before submitting the report.
@acassis
Copy link
Contributor

acassis commented Jan 7, 2025

According to git, this symbol was added by @augustofg sometime ago, but I think the original support to PWM multi-channel was created by other people (my memory state that it was created by you or Matias Nitsche @protobits).

@raiden00pl
Copy link
Member Author

@acassis multichan PWM was added loooong time ago in e8c2466.
I have no idea why it was done this way, but I think it can be done better by avoiding unnecessary complication of the code. To be clear: this code works fine for many years and no one seems to have complained before. This issue aims to find out the community's opinion, whether anyone has any objections to proposed change :)

@michallenc
Copy link
Contributor

Hi all, just please note that this might be a breaking API change, since it is safe to assume applications are using CONFIG_PWM_MULTICHAN to determine how to access pwm_info_s structure. That said, I completely agree with the removal of CONFIG_PWM_MULTICHAN option.

I have proposed other breaking change several months ago in #12381 that simplifies pwm_info_s and would possibly also make CONFIG_PWM_MULTICHAN obsolete. I have not done much work on it as it is not my priority now, just some initial tests with SAMv7 build, you can see this commit michallenc@8224f0e (though I still used multichan config there, but it shouldn't be problem to remove it). We would basically shrink the entire PWM structure to

#ifdef CONFIG_PWM_NCHANNELS
#define PWM_NCHANNELS CONFIG_PWM_NCHANNELS
#else
#define PWM_NCHANNELS 1
#endif

struct pwm_chan_s  channels[PWM_NCHANNELS];
FAR void           *arg; 

There were also some other proposals in the issue I haven't worked on yet, this is just an initial kick off.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants