drivers: pwm: siwx91x: Add siwx91x PWM driver #86056
Conversation
zephyrbot
added
platform: Silabs
zephyrbot
requested review from
anangl,
asmellby,
gmarull,
henrikbrixandersen,
jerome-pouiller,
jhedberg,
Martinhoff-maker,
mnkp,
nordic-krch,
rettichschnidi and
yonsch
|
The following west manifest projects have changed revision in this Pull Request:
✅ All manifest checks OK Note: This message is automatically posted and updated by the Manifest GitHub Action. |
zephyrbot
added
manifest
manifest-hal_silabs
DNM (manifest)
drivers/pwm/pwm_silabs_siwx91x.c
Outdated
| data->pwm_channel_config[channel].is_polarity_high)) { | ||
| return -EINVAL; | ||
| } | ||
| data->pwm_polarity = flags; |
There was a problem hiding this comment.
data->pwm_polarity is shared between all the channels but I believe it shouldn't.
BTW, this field is probably redundant with is_polarity_(high|low):
bool polarity_inverted = (flags == PWM_POLARITY_INVERTED);
if (data->pwm_channel_config[channel].polarity_inverted != polarity_inverted)) {
data->pwm_channel_config[channel].polarity_inverted = polarity_inverted;
ret = sl_si91x_pwm_set_output_polarity(!polarity_inverted, polarity_inverted);
}
There was a problem hiding this comment.
Addressed and updated accordingly, but the polarity is the same for all the channels and cannot be independent for each channel (HW limitation). Added a comment for this constraint.
There was a problem hiding this comment.
I am afraid it confuse the user. Don't you think we should check all the enabled active has the same polarity before enabling a new one?
There was a problem hiding this comment.
Yes, I guess all the channels can have the same polarity by removing the flags cell in yaml file and adding the polarity as a new and common attribute for PWM channels in yaml,
silabs,pwm_polarity:
type: int
description: |
Polarity of all the PWM channels
0 - Normal polarity
1 - Inverted polarity
This will allow user to know that all the channels share the same polarity, Your thoughts?
There was a problem hiding this comment.
Then, we check the flags match the DT?
Yes, it's a good idea. I suggest to do the simplest thing. It's fine as soon as the behavior is easy to understand.
There was a problem hiding this comment.
Yes, validation of flags is done, and the code is updated accordingly.
smalae
force-pushed
the
silabs_siwx91x_pwm_driver
branch
from
719b509 to
731bf46
Compare
drivers/pwm/pwm_silabs_siwx91x.c
Outdated
| /* Function to convert prescaler value to a programmable reg value */ | ||
| static int siwx91x_prescale_convert(uint8_t prescale) | ||
| { | ||
|
|
There was a problem hiding this comment.
This empty line should not be there.
drivers/pwm/pwm_silabs_siwx91x.c
Outdated
| data->pwm_channel_config[channel].is_polarity_high)) { | ||
| return -EINVAL; | ||
| } | ||
| data->pwm_polarity = flags; |
There was a problem hiding this comment.
I am afraid it confuse the user. Don't you think we should check all the enabled active has the same polarity before enabling a new one?
smalae
force-pushed
the
silabs_siwx91x_pwm_driver
branch
2 times, most recently
from
c991b6b to
ef0ba91
Compare
|
@smalae can you rebase your branch and update the reference to |
Change PWM pin function names to align with wiseconnect 3.4.0 Signed-off-by: Sai Santhosh Malae <[email protected]>
Implement PWM driver for siwx91x device Signed-off-by: Sai Santhosh Malae <[email protected]>
smalae
force-pushed
the
silabs_siwx91x_pwm_driver
branch
from
ef0ba91 to
2930a51
Compare
Done |
zephyrbot
removed
the
DNM (manifest)
Implement PWM driver for siwx91x device