spi: shell: rework command to use SPI devices #73962

liambeguin · 2024-06-08T19:57:03Z

Rework the shell interface to work on SPI devices. This will create a
build-time list of all spi device nodes based on the devicetree.

At runtime, the shell will autocomplete device names and use
spi_transceive_dt() to leverage the devicetree configuration of each
node. This removes the need for a conf command, and will ensure that all
chip select modes are properly supported (i.e. cs-gpios).

github-actions · 2024-06-08T19:57:45Z

Hello @liambeguin, and thank you very much for your first pull request to the Zephyr project!
Our Continuous Integration pipeline will execute a series of checks on your Pull Request commit messages and code, and you are expected to address any failures by updating the PR. Please take a look at our commit message guidelines to find out how to format your commit messages, and at our contribution workflow to understand how to update your Pull Request. If you haven't already, please make sure to review the project's Contributor Expectations and update (by amending and force-pushing the commits) your pull request if necessary.
If you are stuck or need help please join us on Discord and ask your question there. Additionally, you can escalate the review when applicable. 😊

tbursztyka · 2024-06-10T06:32:59Z

drivers/spi/spi_shell.c

@@ -74,82 +91,12 @@ static int cmd_spi_transceive(const struct shell *ctx, size_t argc, char **argv)
 	return ret;
 }

-static int cmd_spi_conf(const struct shell *ctx, size_t argc, char **argv)


The idea with this command was to test things at run-time without any DTS pre-configuration made. An on-the-fly configuration command then.

Note that, however, having something like you propose co-existing with this spi_conf, would be nice.

The idea with this command was to test things at run-time without any DTS pre-configuration made. An on-the-fly configuration command then.

I originally had this as an extra command, but thought it doesn't really take much effort to add the node to an existing devicetree.

Note that, however, having something like you propose co-existing with this spi_conf, would be nice.

Agreed, to me this is very useful for device driver development, and also ensures proper CS handling.

I don't mind adding back the conf command though, but I think we should be explicit about the CS handling.

We could also have a build CONFIG_ to switch between both modes, one interacting with SPI controllers, and another with SPI devices?

I originally had this as an extra command, but thought it doesn't really take much effort to add the node to an existing devicetree.

True, and I wouldn't see myself doing otherwise.

@benner any input on this one?

I like this approach because it allows to use different chip selects. During development I did knew how to do something like IS_SPIDEV_NODE and I was lucky that in my case it was the only one device on SPI bus.

We could also have a build CONFIG_ to switch between both modes, one interacting with SPI controllers, and another with SPI devices?

I agree this is better option

benner · 2024-06-20T14:18:10Z

I also found one retrogression (at least for me). I'm getting

bin/../lib/gcc/arm-zephyr-eabi/12.2.0/../../../../arm-zephyr-eabi/bin/ld.bfd: zephyr/drivers/spi/libdrivers__spi.a(spi_shell.c.obj):(.data.spidev_list+0x54): undefined reference to `__device_dts_ord_191'
collect2: error: ld returned 1 exit status

After some digging I found one SPI bus device with fake compatible (I was too lazy to create proper bindings).

	x: x_0@0 {
		compatible = "vnd,spi-device";
		reg = <0>;
		spi-max-frequency = <DT_FREQ_K(2500)>;
	};

If I'm disabling it (ant ton of code accessing it) - everything is OK.

Not sure how it can be related because I'm using it with same const struct spi_dt_spec/SPI_DT_SPEC_GET API.

liambeguin · 2024-06-20T15:31:51Z

I also found one retrogression (at least for me). I'm getting
bin/../lib/gcc/arm-zephyr-eabi/12.2.0/../../../../arm-zephyr-eabi/bin/ld.bfd: zephyr/drivers/spi/libdrivers__spi.a(spi_shell.c.obj):(.data.spidev_list+0x54): undefined reference to `__device_dts_ord_191'
collect2: error: ld returned 1 exit status
After some digging I found one SPI bus device with fake compatible (I was too lazy to create proper bindings).
	x: x_0@0 {
		compatible = "vnd,spi-device";
		reg = <0>;
		spi-max-frequency = <DT_FREQ_K(2500)>;
	};
If I'm disabling it (ant ton of code accessing it) - everything is OK.

Not sure how it can be related because I'm using it with same const struct spi_dt_spec/SPI_DT_SPEC_GET API.

I think this comes from the fact that I add the device to a list but because you don't have a valid compatible it gets deleted later on. I'm not entirely sure, but I noticed something similar with a valid compatible and its config disabled.

benner · 2024-06-20T15:40:26Z

@liambeguin, this my assumption too. I quickly described simple dts:

% cat dts/bindings/spi/astro,spi.yaml  
description: foo
compatible: "astro,spi"
include:
  - spi-device.yaml

and changed compatable string inside overlay - problem persist.

So please ignore this regression for now. I will play more.

Problem - I'm on PTO, so not much time I have ATM.

Overall code and idea - looks good.

benner · 2024-07-03T19:21:46Z

I know it's rc2 now. Can f451e54 be extracted to separate PR and merged (it's kind fix and fits current stage)?

liambeguin · 2024-07-03T20:15:45Z

I know it's rc2 now. Can f451e54 be extracted to separate PR and merged (it's kind fix and fits current stage)?

sure, I don't mind separating it if someone is ready to take it

benner · 2024-07-09T18:45:17Z

I made PR with just first commit (commit author preserved): #75685

kartben · 2024-07-09T23:04:45Z

@liambeguin would #71912 be useful/applicable for—at least some of—what you're trying to accomplish here?

liambeguin · 2024-07-10T14:35:49Z

@kartben if I understand correctly that MR is related to node labels no? For the SPI shell, I'm looking at all devices that have a required spi property so I don't think it would apply directly.

github-actions · 2024-09-09T00:32:41Z

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.

tbursztyka · 2024-09-09T13:02:09Z

@liambeguin Could you rebase against the main branch?

@teburd any input? lgtm at least here

liambeguin · 2024-09-09T16:27:37Z

@liambeguin Could you rebase against the main branch?

Will do!

Rework the shell interface to work on SPI devices. This will create a build-time list of all spi device nodes based on the devicetree. At runtime, the shell will autocomplete device names and use spi_transceive_dt() to leverage the devicetree configuration of each node. This removes the need for a conf command, and will ensure that all chip select modes are properly supported (i.e. cs-gpios). Signed-off-by: Liam Beguin <[email protected]>

zephyrbot added the area: SPI SPI bus label Jun 8, 2024

zephyrbot requested review from tbursztyka and teburd June 8, 2024 19:57

zephyrbot assigned tbursztyka Jun 8, 2024

tbursztyka requested changes Jun 10, 2024

View reviewed changes

tbursztyka previously approved these changes Jun 27, 2024

View reviewed changes

benner approved these changes Jun 27, 2024

View reviewed changes

benner mentioned this pull request Jul 9, 2024

spi: shell: add missing newline character #75685

Merged

github-actions bot added the Stale label Sep 9, 2024

liambeguin dismissed tbursztyka’s stale review via 4c06a04 September 9, 2024 19:37

liambeguin force-pushed the lvb/spi-shell branch from 6a921d4 to 4c06a04 Compare September 9, 2024 19:37

github-actions bot removed the Stale label Sep 10, 2024

tbursztyka approved these changes Sep 10, 2024

View reviewed changes

liambeguin force-pushed the lvb/spi-shell branch from 4c06a04 to af5a532 Compare September 10, 2024 20:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

spi: shell: rework command to use SPI devices #73962

spi: shell: rework command to use SPI devices #73962

liambeguin commented Jun 8, 2024

github-actions bot commented Jun 8, 2024

tbursztyka Jun 10, 2024

liambeguin Jun 10, 2024

tbursztyka Jun 10, 2024

benner Jun 20, 2024

benner commented Jun 20, 2024

liambeguin commented Jun 20, 2024

benner commented Jun 20, 2024

benner commented Jul 3, 2024

liambeguin commented Jul 3, 2024

benner commented Jul 9, 2024

kartben commented Jul 9, 2024 •

edited

Loading

liambeguin commented Jul 10, 2024

github-actions bot commented Sep 9, 2024

tbursztyka commented Sep 9, 2024

liambeguin commented Sep 9, 2024

spi: shell: rework command to use SPI devices #73962

Are you sure you want to change the base?

spi: shell: rework command to use SPI devices #73962

Conversation

liambeguin commented Jun 8, 2024

github-actions bot commented Jun 8, 2024

tbursztyka Jun 10, 2024

Choose a reason for hiding this comment

liambeguin Jun 10, 2024

Choose a reason for hiding this comment

tbursztyka Jun 10, 2024

Choose a reason for hiding this comment

benner Jun 20, 2024

Choose a reason for hiding this comment

benner commented Jun 20, 2024

liambeguin commented Jun 20, 2024

benner commented Jun 20, 2024

benner commented Jul 3, 2024

liambeguin commented Jul 3, 2024

benner commented Jul 9, 2024

kartben commented Jul 9, 2024 • edited Loading

liambeguin commented Jul 10, 2024

github-actions bot commented Sep 9, 2024

tbursztyka commented Sep 9, 2024

liambeguin commented Sep 9, 2024

kartben commented Jul 9, 2024 •

edited

Loading