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

NXP S32 introduce support PSI5 #79824

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

NXP S32 introduce support PSI5 #79824

wants to merge 3 commits into from
+1,466 −6

Conversation

congnguyenhuu
Copy link
Contributor

@congnguyenhuu congnguyenhuu commented Oct 15, 2024 •
edited
Loading

Introduce support Peripheral Sensor Interface (PSI5) driver on S32Z. This driver allows to communication (send, receive) with PSI5 device.
The test result: PSI5-TESTING-RESULT.pptx

@congnguyenhuu congnguyenhuu changed the title Nxp S32 introduce support psi5 NXP S32 introduce support PSI5 Oct 15, 2024
@congnguyenhuu congnguyenhuu force-pushed the nxp-s32-introduce-support-psi5 branch from 0ee0652 to b50ef76 Compare October 15, 2024 06:00
@zephyrbot
Copy link
Collaborator

zephyrbot commented Oct 15, 2024 •
edited
Loading

The following west manifest projects have changed revision in this Pull Request:

Name Old Revision New Revision Diff
hal_nxp zephyrproject-rtos/hal_nxp@c15dd51 zephyrproject-rtos/hal_nxp#451 zephyrproject-rtos/hal_nxp#451/files

⛔ DNM label due to: 1 project with PR revision

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@zephyrbot zephyrbot added manifest manifest-hal_nxp DNM This PR should not be merged (Do Not Merge) labels Oct 15, 2024
@congnguyenhuu congnguyenhuu force-pushed the nxp-s32-introduce-support-psi5 branch 2 times, most recently from acc9b3e to 5a072e9 Compare October 24, 2024 07:20
@congnguyenhuu congnguyenhuu force-pushed the nxp-s32-introduce-support-psi5 branch 3 times, most recently from 101fad8 to dfebc33 Compare November 1, 2024 02:41
@congnguyenhuu congnguyenhuu force-pushed the nxp-s32-introduce-support-psi5 branch from dfebc33 to 69e051a Compare November 22, 2024 10:27
@congnguyenhuu congnguyenhuu force-pushed the nxp-s32-introduce-support-psi5 branch 3 times, most recently from 402551c to 9a0367b Compare December 5, 2024 07:52
@congnguyenhuu congnguyenhuu force-pushed the nxp-s32-introduce-support-psi5 branch 3 times, most recently from 4ce3d57 to eca2db8 Compare December 12, 2024 07:58
@congnguyenhuu congnguyenhuu force-pushed the nxp-s32-introduce-support-psi5 branch 4 times, most recently from 147bb43 to 05f2644 Compare January 10, 2025 09:43
@congnguyenhuu congnguyenhuu mentioned this pull request Jan 15, 2025
Open
@congnguyenhuu congnguyenhuu marked this pull request as ready for review January 15, 2025 03:15
@manuargue manuargue added the RFC Request For Comments: want input from the community label Jan 15, 2025
@congnguyenhuu congnguyenhuu force-pushed the nxp-s32-introduce-support-psi5 branch from 05f2644 to 942502a Compare January 15, 2025 07:56
JarmouniA
JarmouniA reviewed Jan 15, 2025
View reviewed changes
Copy link
Collaborator

@JarmouniA JarmouniA left a comment

Choose a reason for hiding this comment

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

A test is needed as well, samples are not a replacement for tests.

dts/bindings/psi5/psi5-controller.yaml Outdated
#
# SPDX-License-Identifier: Apache-2.0

description: Properties for Peripheral Sensor Interface (PSI5)
Copy link
Collaborator

Choose a reason for hiding this comment

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

DeviceTree binding for

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated

Copy link
Collaborator

Choose a reason for hiding this comment

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

@congnguyenhuu They are redundant and do not need to be prefixed. Putting them in dts/bindings already indicates that they are a binding. E.g. other bindings description.

Renesas RA ADC node

Static Battery Characteristics

Description of pre-defined file systems.

ADI MAX32 compatible Counter RTC

PL330 DMA Controller

samples/boards/nxp/s32/psi5/src/main.c Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since this is a vendor-agnostic API, I don't see why this sample is placed in the NXP folder.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Because this is the smallest sample it provides that can run, I guess its prototype is developed based on nxp soc.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Except the device node name (added a comment about it below), I don't see anything vendor-specific in the sample's code, that would justify placing it in the vendor's folder.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved this sample to a generic driver sample.
And also created a test API driver .

JarmouniA
JarmouniA reviewed Jan 15, 2025
View reviewed changes
samples/boards/nxp/s32/psi5/src/main.c Outdated

#include <zephyr/drivers/psi5/psi5.h>

#define PSI5_NODE DT_INST(0, nxp_s32_psi5)
Copy link
Collaborator

Choose a reason for hiding this comment

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

A vendor-agnostic alias, that will be defined in board overlay, should be used here, instead of the IP DT node name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I updated

@JarmouniA JarmouniA mentioned this pull request Jan 15, 2025
Open
@congnguyenhuu
drivers: introduce support Peripheral Sensor Interface (PSI5) driver
2d71628 
This driver allows to communication (send, receive) with PSI5 device

Signed-off-by: Cong Nguyen Huu <cong.nguyenhuu@nxp.com>
@congnguyenhuu
boards: s32z270: enable support psi5
3b4bec4 
enable support psi5

Signed-off-by: Cong Nguyen Huu <cong.nguyenhuu@nxp.com>
@congnguyenhuu
tests/samples: drivers: create test, sample for psi5 driver
8c5a2c3 
Create test, sample for psi5 driver

Signed-off-by: Cong Nguyen Huu <cong.nguyenhuu@nxp.com>
@congnguyenhuu congnguyenhuu force-pushed the nxp-s32-introduce-support-psi5 branch from 942502a to 8c5a2c3 Compare January 16, 2025 07:00
@nashif nashif assigned manuargue and unassigned nashif and MaureenHelm Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JarmouniA JarmouniA JarmouniA left review comments

@bperseghetti bperseghetti Awaiting requested review from bperseghetti

@Dat-NguyenDuy Dat-NguyenDuy Awaiting requested review from Dat-NguyenDuy

@decsny decsny Awaiting requested review from decsny

@galak galak Awaiting requested review from galak

@kartben kartben Awaiting requested review from kartben

@manuargue manuargue Awaiting requested review from manuargue

@MaureenHelm MaureenHelm Awaiting requested review from MaureenHelm

@nashif nashif Awaiting requested review from nashif

@PetervdPerk-NXP PetervdPerk-NXP Awaiting requested review from PetervdPerk-NXP

@rruuaanng rruuaanng Awaiting requested review from rruuaanng

@stephanosio stephanosio Awaiting requested review from stephanosio

At least 2 approving reviews are required to merge this pull request.

Assignees

@manuargue manuargue

Labels
area: Devicetree area: Process area: Samples Samples DNM This PR should not be merged (Do Not Merge) manifest manifest-hal_nxp platform: NXP S32 NXP Semiconductors, S32 RFC Request For Comments: want input from the community
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@congnguyenhuu @zephyrbot @JarmouniA @rruuaanng @nashif @manuargue @MaureenHelm