drivers: pwm: rpi_pico: Configuring the divide ratio adaptively #78786
Conversation
soburi
force-pushed
the
improve_rpi_pico_pwm
branch
from
6cd6a6c
to
6b743bf
Compare
zephyrbot
added
platform: Raspberry Pi Pico
zephyrbot
requested review from
anangl,
bbilas,
dkalowsk,
henrikbrixandersen,
kartben,
Mani-Sadhasivam,
mmahadevan108,
nashif and
simonguinot
soburi
force-pushed
the
improve_rpi_pico_pwm
branch
from
6b743bf
to
258ac05
Compare
| @@ -16,88 +16,72 @@ properties: | |||
|
|
|||
| divider-int-0: | |||
| type: int | |||
| default: 1 | |||
| description: | | |||
| The integral part of the divider for pwm slice 0. | |||
| This number should be in the range 1 - 255. Defaults | |||
There was a problem hiding this comment.
Does it still default to the reset value?
There was a problem hiding this comment.
Yes, it does. It is set in this part.
soburi
force-pushed
the
improve_rpi_pico_pwm
branch
from
258ac05
to
bd00fd8
Compare
There was a problem hiding this comment.
Hi @soburi,
The LED part looks OK but I am not sure about the PWM driver. Please find some questions and suggestions below.
drivers/pwm/pwm_rpi_pico.c
Outdated
| #define PWM_INST_RPI_SLICE_DIVIDER(idx, n) \ | ||
| { \ | ||
| .integral = DT_INST_PROP_OR(idx, UTIL_CAT(divider_int_, n), 0), \ | ||
| .frac = DT_INST_PROP_OR(idx, UTIL_CAT(divider_frac_, n), 0), \ |
There was a problem hiding this comment.
You use trailing spaces for the lines above but tabs for those below (PWM_RPI_INIT).
| @@ -16,88 +16,72 @@ properties: | |||
|
|
|||
| divider-int-0: | |||
There was a problem hiding this comment.
Out of curiosity what is the interest for a user to manually set/force the divider value of a PWM ? After all a user only wants ->set_cycles() to succeed. If the divider is set/forced, I suspect this limits the supported cycle range (period and pulse)...
There was a problem hiding this comment.
Your understanding is correct.
This may be useful when creating deterministically precise waveforms, but I don't think it's necessary for most use cases.
However, since this is the current specification, it must be maintained.
This PR intends to relax the limitation.
| @@ -16,88 +16,72 @@ properties: | |||
|
|
|||
| divider-int-0: | |||
| type: int | |||
| default: 1 | |||
| description: | | |||
| The integral part of the divider for pwm slice 0. | |||
| This number should be in the range 1 - 255. Defaults | |||
There was a problem hiding this comment.
Actually 0 is also a supported/valid value for the driver. It means "automatic divider handling". Honestly, I find the whole thing confusing... It's not clear that not setting this value or setting it to 0 leaves the driver to take care of everything. Also we are kind of hijacking a device-tree hardware attribute by using an unsupported value to control a special behavior of the driver.
There was a problem hiding this comment.
I forgot to update the description.
The "automatic divider handling" behavior is extended in this PR.
I added a description of this feature.
samples/drivers/led/pwm/Kconfig
Outdated
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| config BLINK_DURATION_SHORT | ||
| int "LED on and off times for short-cycle blinking demo" |
There was a problem hiding this comment.
Suggestion: Blinking delay for short cycle demo`.
There was a problem hiding this comment.
Thank you for the suggestion. I applied it.
samples/drivers/led/pwm/Kconfig
Outdated
| int "LED on and off times for short-cycle blinking demo" | ||
| default 100 | ||
| help | ||
| Specifies the LED blinking time in milliseconds in the short-cycle |
There was a problem hiding this comment.
And maybe: Specifies the LED on/off delay in milliseconds for short cycle blinking demonstration.
samples/drivers/led/pwm/Kconfig
Outdated
| # Copyright (c) 2024 TOKITA Hiroshi | ||
| # SPDX-License-Identifier: Apache-2.0 | ||
|
|
||
| config BLINK_DURATION_SHORT |
There was a problem hiding this comment.
Suggestion: BLINK_DELAY_SHORT
samples/drivers/led/pwm/Kconfig
Outdated
| Specifies the delay time for the fade demo of the PWM-LED sample. | ||
| The fade demo increases the brightness by 1 percent point every | ||
| time this delay time elapses and ends when it reaches the value | ||
| of MAX_BRIGHTNESS. |
There was a problem hiding this comment.
Suggestion: The brightness gradually increases by one level each time this delay elapses.
Let's not talk about percent. When we will update the maximum brightness to 255, we might forget to update this comment :)
There was a problem hiding this comment.
I applied it.
Let's not talk about percent. When we will update the maximum brightness to 255, we might forget to update this comment :)
OK. I got it.
samples/drivers/led/pwm/Kconfig
Outdated
| int "delay time for fade demo" | ||
| default 10 | ||
| help | ||
| Specifies the delay time for the fade demo of the PWM-LED sample. |
There was a problem hiding this comment.
Suggestion: Specifies the delay in milliseconds for ...
samples/drivers/led/pwm/src/main.c
Outdated
| err = led_blink(led_pwm, led, 100, 100); | ||
| #if CONFIG_BLINK_DURATION_SHORT > 0 | ||
| /* Start LED blinking (short cycle) */ | ||
| err = led_blink(led_pwm, led, CONFIG_BLINK_DURATION_SHORT, CONFIG_BLINK_DURATION_SHORT); |
There was a problem hiding this comment.
Optionally, limit the line length to 80 columns, to stay consistent with the rest of the code.
There was a problem hiding this comment.
I want to conform basically to the clang-format settings.
Line 31 in 715b973
If the `divider-int-0` or variations of these for each channel properties are not specified, or if these is 0, the driver dynamically configures the division ratio by specified cycles. The driver will operate at the specified division ratio if a non-zero value is specified for `divider-int-0`. This is unchanged from previous behavior. Please specify ``divider-int-0`` explicitly to make the same behavior as before. In addition, the default device tree properties related to the division ratio have been removed. Signed-off-by: TOKITA Hiroshi <[email protected]>
soburi
force-pushed
the
improve_rpi_pico_pwm
branch
from
bd00fd8
to
c09b626
Compare
Make the blinking and fading parameters adjustable so that samples can be run on devices that support only short periods. Signed-off-by: TOKITA Hiroshi <[email protected]>
Tweaking the parameters to go the sample without any errors. Signed-off-by: TOKITA Hiroshi <[email protected]>
Add test configuration for rpi_pico Signed-off-by: TOKITA Hiroshi <[email protected]>
soburi
force-pushed
the
improve_rpi_pico_pwm
branch
from
c09b626
to
e8d315f
Compare
As with many other implementations, we should set the division ratio to a value that corresponds to the interval specified in the API.
If the
divider-int-0or variations of these for each channel propertiesare not specified, or if these is 0,
the driver dynamically configures the division ratio by specified cycles.
The driver will operate at the specified division ratio if a non-zero
value is specified for
divider-int-0.This is unchanged from previous behavior.
Please specify
divider-int-0explicitly to make the same behavior asbefore.
In addition, the default device tree properties related to the division
ratio have been removed.