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.

Sign up for GitHub

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

Jump to bottom

drivers: pwm: bbled: bbled-pwm driver to compatible with led api #61748

Merged
merged 2 commits into from
Mar 3, 2024
Merged

drivers: pwm: bbled: bbled-pwm driver to compatible with led api #61748

merged 2 commits into from
Mar 3, 2024

Conversation

Manimaran-A
Copy link
Contributor

Zephyr has a PWM-LED driver which makes calls to a PWM driver to control the pin. The previous MCHP PWM-BBLED driver do not report its actual PWM cycles, instead it reported the clock source it used: 32KHz or 48MHz. This bug caused the LED-PWM driver set brightness to compute incorrect parameters passed to the PWM-BBLED driver. The BBLED hardware has a built-in divider of 256 therefore its maximum cycles are 128 and 48M/256 respectively. This change also simplified the calculations. A DT overlay for the MEC172x EVB was added to the PWM-LED sample to demonstrate driver operation. We found specifying >= 50 ms for the PWM period in the device tree nodes provided a good operating range for the LED-PWM driver parameter calculations. Note: BBLED hardware implements an 8-bit PWM with a fixed divider of 256 and a 12-bit frequency pre-scaler. Duty cycle is represented by an 8-bit value.

@zephyrbot zephyrbot added area: LED Label to identify LED subsystem area: PWM Pulse Width Modulation area: Devicetree Binding PR modifies or adds a Device Tree binding platform: Microchip MEC Microchip MEC Platform labels Aug 22, 2023
@Manimaran-A
Copy link
Contributor Author

@simonguinot @henrikbrixandersen Could you please help to review?

@simonguinot
Copy link
Collaborator

@simonguinot @henrikbrixandersen Could you please help to review?

Hi @Manimaran-A. I'll have a look at it this week-end.

@simonguinot
Copy link
Collaborator

simonguinot commented Sep 10, 2023 •
edited
Loading

Hi @Manimaran-A. I apologize for the delay. Unfortunately I am not familiar enough with both the PWM subsystem and the MCHP PWM BBLED controller to help with this review.

I can see the division by 256 you added in the get_cycles_per_sec API function. And it looks right. But there are also other changes in the driver code. I don't understand them and they are not explained in the commit message. And I am not comfortable with approving changes that I don't fully understand. But I am sure that the PWM guys will be able to help with the review.

In addition I think you probably should move the addition of the DT overlay files into a dedicated commit.

@albertofloyd
Copy link
Collaborator

@Manimaran-A do split the commit in two: one for dts changes, one driver changes

@Manimaran-A Manimaran-A force-pushed the xec_bbledpwm branch 4 times, most recently from a94cd5e to 51633fe Compare September 27, 2023 07:33
@MaureenHelm
Copy link
Member

@jvasanth1 please take a look

jvasanth1
jvasanth1 previously approved these changes Oct 26, 2023
View reviewed changes
@github-actions GitHub Actions
Copy link

This pull request has been marked as stale because it has been open (more than) 60 days with no activity. Remove the stale label or add a comment saying that you would like to have the label removed otherwise this pull request will automatically be closed in 14 days. Note, that you can always re-open a closed pull request at any time.

@github-actions github-actions bot added the Stale label Dec 26, 2023
@github-actions github-actions bot closed this Jan 9, 2024
@simonguinot simonguinot reopened this Jan 9, 2024
@zephyrbot zephyrbot added the area: Samples Samples label Jan 9, 2024
@zephyrbot zephyrbot requested a review from kartben January 9, 2024 14:06
@simonguinot
Copy link
Collaborator

Hi @jvasanth1 and @Manimaran-A,

We need to merge this PR in order to remove the led_mchp_xec driver. The generic led_pwm driver should be used instead. Is that possible to get the PWM folks to review it ? @henrikbrixandersen, @anangl, can you help ?

@jvasanth1
Copy link
Collaborator

@albertofloyd can you help to review and approve?

@github-actions github-actions bot removed the Stale label Jan 10, 2024
albertofloyd
albertofloyd previously approved these changes Feb 9, 2024
View reviewed changes
fabiobaltieri
fabiobaltieri reviewed Feb 9, 2024
View reviewed changes
samples/drivers/led_pwm/boards/mec15xxevb_assy6853.overlay Outdated

&pinctrl {
led0_gpio156_invert: led0_gpio156_invert {
pinmux = < MCHP_XEC_PINMUX(0156, MCHP_AF1) >;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pinmux = < MCHP_XEC_PINMUX(0156, MCHP_AF1) >;
pinmux = <MCHP_XEC_PINMUX(0156, MCHP_AF1)>;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, updated.

samples/drivers/led_pwm/boards/mec15xxevb_assy6853.overlay Outdated
microchip,output-func-invert;
};
led1_gpio157_invert: led1_gpio157_invert {
pinmux = < MCHP_XEC_PINMUX(0157, MCHP_AF1) >;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pinmux = < MCHP_XEC_PINMUX(0157, MCHP_AF1) >;
pinmux = <MCHP_XEC_PINMUX(0157, MCHP_AF1)>;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

samples/drivers/led_pwm/boards/mec15xxevb_assy6853.overlay Outdated
microchip,output-func-invert;
};
led2_gpio153_invert: led2_gpio153_invert {
pinmux = < MCHP_XEC_PINMUX(0153, MCHP_AF1) >;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pinmux = < MCHP_XEC_PINMUX(0153, MCHP_AF1) >;
pinmux = <MCHP_XEC_PINMUX(0153, MCHP_AF1)>;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the comments.

samples/drivers/led_pwm/boards/mec172xevb_assy6906.overlay Outdated

&pinctrl {
led0_gpio156_invert: led0_gpio156_invert {
pinmux = < MCHP_XEC_PINMUX(0156, MCHP_AF1) >;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pinmux = < MCHP_XEC_PINMUX(0156, MCHP_AF1) >;
pinmux = <MCHP_XEC_PINMUX(0156, MCHP_AF1)>;

etc... you got the idea :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated.

@fabiobaltieri fabiobaltieri added this to the v3.7.0 milestone Feb 9, 2024
@Manimaran-A Manimaran-A dismissed stale reviews from albertofloyd and jvasanth1 via 1010143 February 23, 2024 04:36
@Manimaran-A
drivers: pwm: bbled: Update bbled-pwm, to be compatible with PWM-LED
78682eb 
Zephyr has a PWM-LED driver which makes calls to a PWM driver to control
the pin. The previous MCHP PWM-BBLED driver do not report its actual
PWM cycles, instead it reported the clock source it used: 32KHz or 48MHz.
This bug caused the LED-PWM driver set brightness to compute incorrect
parameters passed to the PWM-BBLED driver. The BBLED hardware has a
built-in divider of 256 therefore its maximum cycles are 128 and 48M/256
respectively. This change also simplified the calculations. A DT overlay
for the MEC172x EVB was added to the PWM-LED sample to demonstrate driver
operation. We found specifying >= 50 ms for the PWM period in the device
tree nodes provided a good operating range for the LED-PWM driver
parameter calculations.
Note: BBLED hardware implements an 8-bit PWM with a fixed divider of 256
and a 12-bit frequency pre-scaler. Duty cycle is represented by an 8-bit
value.

Signed-off-by: Manimaran A <manimaran.a@microchip.com>
@Manimaran-A
drivers: pwm: bbled: Update bbled-pwm dts files
8594eeb 
Updated Overlay, dtsi and yaml files.

Signed-off-by: Manimaran A <manimaran.a@microchip.com>
@aescolar aescolar added the hwmv2-likely-conflict DNM until collab-hwmv2 has been merged label Feb 27, 2024
@carlescufi carlescufi merged commit 28475a3 into zephyrproject-rtos:main Mar 3, 2024
23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jvasanth1 jvasanth1 jvasanth1 approved these changes

@fabiobaltieri fabiobaltieri fabiobaltieri approved these changes

@nashif nashif Awaiting requested review from nashif

@galak galak Awaiting requested review from galak

@anangl anangl Awaiting requested review from anangl

@bbilas bbilas Awaiting requested review from bbilas

@henrikbrixandersen henrikbrixandersen Awaiting requested review from henrikbrixandersen

@Mani-Sadhasivam Mani-Sadhasivam Awaiting requested review from Mani-Sadhasivam

@simonguinot simonguinot Awaiting requested review from simonguinot

@VenkatKotakonda VenkatKotakonda Awaiting requested review from VenkatKotakonda

@kartben kartben Awaiting requested review from kartben

@albertofloyd albertofloyd Awaiting requested review from albertofloyd

Assignees

@jvasanth1 jvasanth1

Labels
area: Devicetree Binding PR modifies or adds a Device Tree binding area: LED Label to identify LED subsystem area: PWM Pulse Width Modulation area: Samples Samples hwmv2-likely-conflict DNM until collab-hwmv2 has been merged platform: Microchip MEC Microchip MEC Platform
Projects
Dev Review
Status: Done
Milestone
v3.7.0
Development

Successfully merging this pull request may close these issues.
9 participants
@Manimaran-A @simonguinot @albertofloyd @MaureenHelm @jvasanth1 @fabiobaltieri @carlescufi @zephyrbot @aescolar