spi: shell: rework command to use SPI devices

[liambeguin]

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]

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]

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.

[liambeguin]

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?

[tbursztyka]

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?

[benner]

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]

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]

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]

@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]

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

[liambeguin]

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]

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

[kartben]

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

[liambeguin]

@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]

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]

@liambeguin Could you rebase against the main branch?

@teburd any input? lgtm at least here

[liambeguin]

@liambeguin Could you rebase against the main branch?

Will do!

[liambeguin]

Just rebased again on the latest branch. Any other inputs on this MR?

[yishai1999]

@liambeguin Any movement on this?

[liambeguin]

@liambeguin Any movement on this?

I've been rebasing the changes, but haven't heard back from reviewers.

[yishai1999]

@tbursztyka can this be merged?

[tbursztyka]

@tbursztyka can this be merged?

@nashif @kartben ?

[yishai1999]

@tbursztyka Any news?

[kartben]

@liambeguin you would need to solve the merge conflicts please

[github-actions]

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.

[kartben]

@liambeguin you would need to solve the merge conflicts please

@liambeguin would you be able to do this? I will then personally take care that it gets the right approvals. Thanks!

[liambeguin]

@kartben, sure I can have a look. I'll try to get to it in the next couple days

[github-actions]

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.