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

worldsemi,ws2812-(gpio|rpi_pico-pio) pin property change into gpios #68514

Conversation

soburi
Copy link
Member

@soburi soburi commented Feb 5, 2024

ws2812-gpio's in-gpios property is not used as an input pin.
https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/led_strip/ws2812_gpio.c#L239

Renaming it to gpios to reflect the actual situation.

For sharing pin definition property among worldsemi,ws2812-gpio and worldsemi,ws2812-rpi_pico-pio,
rename output-pin to gpios.

@soburi soburi added the DNM This PR should not be merged (Do Not Merge) label Feb 5, 2024
Comment on lines 53 to 58
in-gpios:
type: phandle-array
required: true
description: |
Select the output pin.
GPIO phandle and specifier for the pin connected to the daisy
chain's input pin. Exactly one pin should be given.
Copy link
Collaborator

Choose a reason for hiding this comment

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

This doesn't really make sense, it it still the output pin from the MCU side, it should remain named as output. You wouldn't name the MISO pin on an SPI bus as "output-gpios" because it's an output from the sensor side.

Copy link
Member Author

@soburi soburi Feb 5, 2024

Choose a reason for hiding this comment

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

@nordicjm
Thank you for comment.

I am also aware of the contradictions and will consider them as well.
It's a definition borrowed from ws2812-gpio, and also the original doesn't seem to be good.

https://github.com/zephyrproject-rtos/zephyr/blob/main/drivers/led_strip/ws2812_gpio.c#L239

This code uses in_gpio as output....

I think this is also included the scope for reconsideration.

I was looking at the review request for #67610.
I'm considering to create common-named gpio property between ws2812-gpio and ws2812-rpi-pico-pio for enabling to define common overlay.

I was interrupted and didn't pay attention to the details. Sorry for the confusion.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a more common binding would probably be a good idea. With all of the different variations of the binding and drivers, you almost have to do something like what I did in #67610 did in order to get many boards supported.

Copy link
Member Author

@soburi soburi Feb 5, 2024

Choose a reason for hiding this comment

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

Since we are using a shield, I would like to describe the pins with connector definition.

In this case, such like below,

&bff_led_strip {
	output-pin = <&xiao_d 3 GPIO_ACTIVE_HIGH>;
};

To do that, we need to change the output-pin definition on the ws2812-rpi_pico-gpio side to one based on gpio.
I don't think the change is any major problems with this, and I think it will be a good improvement.

By the way, as shown above, if only the gpio pin is defined in overlay, it seems that ws2812-gpio and ws2812-rpi_pico-pio can share it if the gpio property name is the same.

Therefore, I am thinking of standardizing the pin specifications for ws2812-gpio and ws2812-rpi_pico-pio.
As pointed out, the definition of ws2812-gpio is in-gpios, and the meaning of the name is inconsistent with this one.
I think if I include this in the correction, you will be able to write the definition of shield neatly.

With all of the different variations of the binding and drivers, you almost have to do something like what I did in #67610 did in order to get many boards supported.

Since it is not possible to define a shield more over than "Use pin 3 of the Xiao Header", it is inevitable that the boards/*.overlay included in the shield directory will need to be supported.
Even though, I still think it is better to have a common definition that can be created more clearly than just a placeholder.

@soburi soburi force-pushed the ws2812-rpi-output-pin-to-in-gpio branch 2 times, most recently from 850ff4e to bf7aa14 Compare February 5, 2024 14:37
@soburi soburi changed the title ws2812_rpi_pico_pio output-pin property change to in-gpio worldsemi,ws2812-(gpio|rpi_pico-pio) pin property change into gpios Feb 5, 2024
@soburi soburi marked this pull request as ready for review February 5, 2024 15:16
@zephyrbot zephyrbot added area: Samples Samples area: LED Label to identify LED subsystem area: Devicetree Binding PR modifies or adds a Device Tree binding platform: Raspberry Pi Pico Raspberry Pi Pico (RPi Pico) labels Feb 5, 2024
@soburi soburi removed the DNM This PR should not be merged (Do Not Merge) label Feb 5, 2024
@soburi soburi force-pushed the ws2812-rpi-output-pin-to-in-gpio branch 2 times, most recently from ea113b8 to b60d41f Compare February 5, 2024 21:30
@zephyrbot zephyrbot added the Release Notes To be mentioned in the release notes label Feb 5, 2024
Copy link
Collaborator

@simonguinot simonguinot left a comment

Choose a reason for hiding this comment

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

Hi @soburi,

This change looks good to me and I am not sure why this DT property was named in-gpios in a first place. Maybe @mbolivar-ampere can tell us more ?

In addition this PR breaks backwards compatibility of the worldsemi,ws2812-gpio DT node. And I suspect that a comment in doc/releases/migration-guide-3.6.rst won't cut it :) For example in doc/develop/api/api_lifecycle.rst I read that an RFC issue should be open for a breaking API change. Unfortunately I am not familiar with this procedure. So we need to ask for help here.

And to finish, you'll find my only comment below.

drivers/led_strip/ws2812_rpi_pico_pio.c Show resolved Hide resolved
@soburi
Copy link
Member Author

soburi commented Feb 6, 2024

@simonguinot

Thank you for comment.

In addition this PR breaks backwards compatibility of the worldsemi,ws2812-gpio DT node. And I suspect that a comment in doc/releases/migration-guide-3.6.rst won't cut it :) For example in doc/develop/api/api_lifecycle.rst I read that an RFC issue should be open for a breaking API change. Unfortunately I am not familiar with this procedure. So we need to ask for help here.

@kartben

If you know of any rules that should be followed regarding this, please let me know.

@henrikbrixandersen henrikbrixandersen dismissed their stale review February 28, 2024 23:25

Release notes comment addressed.

@aescolar aescolar added the hwmv2-likely-conflict DNM until collab-hwmv2 has been merged label Feb 29, 2024
Copy link
Contributor

@raveious raveious left a comment

Choose a reason for hiding this comment

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

No new comments that aren't covered by others.

@soburi soburi force-pushed the ws2812-rpi-output-pin-to-in-gpio branch from a981b93 to 9bd5c1c Compare March 2, 2024 22:54
@soburi
Copy link
Member Author

soburi commented Mar 2, 2024

Rebased to after hwmv2 world.

@soburi
Copy link
Member Author

soburi commented Mar 3, 2024

CI error seems not related to this change.
May be somethings broken in hwmv2 changes.

@nordicjm nordicjm dismissed their stale review March 4, 2024 07:50

hwmv2 applied

@nordicjm nordicjm removed the hwmv2-likely-conflict DNM until collab-hwmv2 has been merged label Mar 4, 2024
@nordicjm
Copy link
Collaborator

nordicjm commented Mar 4, 2024

There is an overlay being applied to the network core which it should not because the network core does not have an i2s instance on nrf5340

ws2812-gpio's `in-gpios` property is not used as an input pin.
Renaming it to `gpios` to reflect the actual situation.

Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
For sharing pin definition property with `worldsemi,ws2812-gpio`,
rename `output-pin` to `gpios`.

Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
The esp32s3_devkitm and the nrf5340dk_nrf5340 only have ws2812 in
each configurations that is procpu and cpuapp respectively.
Rename overlay and conf files to avoid building with unnecessary
configurations.

Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
@soburi soburi force-pushed the ws2812-rpi-output-pin-to-in-gpio branch from 9bd5c1c to 20c16e4 Compare March 5, 2024 02:10
@soburi
Copy link
Member Author

soburi commented Mar 5, 2024

@nordicjm

There is an overlay being applied to the network core which it should not because the network core does not have an i2s instance on nrf5340

Thank you for the notification.
I fixed it with renaming the files in the boards.

@soburi soburi requested a review from raveious March 5, 2024 02:57
@henrikbrixandersen henrikbrixandersen merged commit 7e78df8 into zephyrproject-rtos:main Mar 6, 2024
22 checks passed
@soburi soburi deleted the ws2812-rpi-output-pin-to-in-gpio branch March 6, 2024 13:21
soburi added a commit to soburi/zephyr that referenced this pull request Mar 11, 2024
…roller

The dts definition of rpi_pico-pio is divided so that ws2812 can be described
with a common dts definition, and the implementation also follows.

Initially, there was some discussion, but with the introduction of zephyrproject-rtos#68514,
the situation has changed to the point where it is more desirable to
be able to reuse the ws2812 DTS.

Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
soburi added a commit to soburi/zephyr that referenced this pull request Mar 13, 2024
…roller

The dts definition of rpi_pico-pio is divided so that ws2812 can be described
with a common dts definition, and the implementation also follows.

Initially, there was some discussion, but with the introduction of zephyrproject-rtos#68514,
the situation has changed to the point where it is more desirable to
be able to reuse the ws2812 DTS.

Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
soburi added a commit to soburi/zephyr that referenced this pull request Mar 13, 2024
…roller

The dts definition of rpi_pico-pio is divided so that ws2812 can be
described with a common dts definition, and the implementation
also follows.

Initially, there was some discussion, but with the introduction of zephyrproject-rtos#68514,
the situation has changed to the point where it is more desirable to
be able to reuse the ws2812 DTS.

Signed-off-by: TOKITA Hiroshi <tokita.hiroshi@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Devicetree Binding PR modifies or adds a Device Tree binding area: LED Label to identify LED subsystem area: Samples Samples platform: Raspberry Pi Pico Raspberry Pi Pico (RPi Pico) Release Notes To be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants