Skip to content

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

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

Closed
liambeguin wants to merge 1 commit into from
Closed

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
147 changes: 47 additions & 100 deletions drivers/spi/spi_shell.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,42 +12,60 @@
#include <zephyr/shell/shell.h>
#include <zephyr/sys/util.h>

#define TXRX_ARGV_BYTES (1)
#define CONF_ARGV_DEV (1)
#define CONF_ARGV_FREQUENCY (2)
#define CONF_ARGV_SETTINGS (3)
#define TXRX_ARGV_DEV (1)
#define TXRX_ARGV_BYTES (2)

/* Maximum bytes we can write and read at once */
#define MAX_SPI_BYTES MIN((CONFIG_SHELL_ARGC_MAX - TXRX_ARGV_BYTES), 32)

static struct device *spi_device;
static struct spi_config config = {.frequency = 1000000,
.operation = SPI_OP_MODE_MASTER | SPI_WORD_SET(8)};
#define SPIDEV_INST(node_id) \
{ \
.dev = DEVICE_DT_GET(node_id), \
.spi = SPI_DT_SPEC_GET(node_id, SPI_WORD_SET(8) | SPI_OP_MODE_MASTER, 0), \
},

static void device_name_get(size_t idx, struct shell_static_entry *entry)
{
const struct device *dev = shell_device_lookup(idx, "spi");
#define IS_SPIDEV_NODE(node_id) \
COND_CODE_1(DT_NODE_HAS_PROP(node_id, spi_max_frequency), (SPIDEV_INST(node_id)), ())

static struct spidev {
const struct device *dev;
const struct spi_dt_spec spi;

entry->syntax = (dev != NULL) ? dev->name : NULL;
entry->handler = NULL;
entry->help = NULL;
entry->subcmd = NULL;
} spidev_list[] = {DT_FOREACH_STATUS_OKAY_NODE(IS_SPIDEV_NODE)};

static void get_spidev_comp(size_t idx, struct shell_static_entry *entry)
{
if (idx < ARRAY_SIZE(spidev_list)) {
entry->syntax = spidev_list[idx].dev->name;
entry->handler = NULL;
entry->subcmd = NULL;
entry->help = "Select spi device.";
} else {
entry->syntax = NULL;
}
}
SHELL_DYNAMIC_CMD_CREATE(dsub_spidev, get_spidev_comp);

SHELL_DYNAMIC_CMD_CREATE(dsub_device_name, device_name_get);
static struct spidev *get_spidev(const char *device_label)
{
for (int i = 0; i < ARRAY_SIZE(spidev_list); i++) {
if (!strcmp(device_label, spidev_list[i].dev->name)) {
return &spidev_list[i];
}
}

static int cmd_spi_transceive(const struct shell *ctx, size_t argc, char **argv)
/* This will never happen because was prompted by shell */
__ASSERT_NO_MSG(false);
return NULL;
}
static int cmd_spi_transceive_dt(const struct shell *ctx, size_t argc, char **argv)
{
uint8_t rx_buffer[MAX_SPI_BYTES] = {0};
uint8_t tx_buffer[MAX_SPI_BYTES] = {0};

if (spi_device == NULL) {
shell_error(ctx, "SPI device isn't configured. Use `spi conf`");
return -ENODEV;
}
struct spidev *spidev = get_spidev(argv[TXRX_ARGV_DEV]);

int bytes_to_send = argc - TXRX_ARGV_BYTES;

for (int i = 0; i < bytes_to_send; i++) {
tx_buffer[i] = strtol(argv[TXRX_ARGV_BYTES + i], NULL, 16);
}
Expand All @@ -58,8 +76,7 @@ static int cmd_spi_transceive(const struct shell *ctx, size_t argc, char **argv)
const struct spi_buf_set tx_buf_set = {.buffers = &tx_buffers, .count = 1};
const struct spi_buf_set rx_buf_set = {.buffers = &rx_buffers, .count = 1};

int ret = spi_transceive(spi_device, &config, &tx_buf_set, &rx_buf_set);

int ret = spi_transceive_dt(&spidev->spi, &tx_buf_set, &rx_buf_set);
if (ret < 0) {
shell_error(ctx, "spi_transceive returned %d", ret);
return ret;
Expand All @@ -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)
Copy link
Contributor

@tbursztyka tbursztyka Jun 10, 2024

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@liambeguin liambeguin Jun 10, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@tbursztyka tbursztyka Jun 10, 2024

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

@benner benner Jun 20, 2024

Choose a reason for hiding this comment

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

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

{
spi_operation_t operation = SPI_WORD_SET(8) | SPI_OP_MODE_MASTER;

/* warning: initialization discards 'const' qualifier from pointer */
/* target type */
struct device *dev = (struct device *)device_get_binding(argv[CONF_ARGV_DEV]);

if (dev == NULL) {
shell_error(ctx, "device %s not found.", argv[CONF_ARGV_DEV]);
return -ENODEV;
}

uint32_t frequency = strtol(argv[CONF_ARGV_FREQUENCY], NULL, 10);

if (!IN_RANGE(frequency, 100 * 1000, 80 * 1000 * 1000)) {
shell_error(ctx, "frequency must be between 100000 and 80000000");
return -EINVAL;
}

/* no settings */
if (argc == (CONF_ARGV_FREQUENCY + 1)) {
goto out;
}

char *opts = argv[CONF_ARGV_SETTINGS];
bool all_opts_is_valid = true;

while (*opts != '\0') {
switch (*opts) {
case 'o':
operation |= SPI_MODE_CPOL;
break;
case 'h':
operation |= SPI_MODE_CPHA;
break;
case 'l':
operation |= SPI_TRANSFER_LSB;
break;
case 'T':
operation |= SPI_FRAME_FORMAT_TI;
break;
default:
all_opts_is_valid = false;
shell_error(ctx, "invalid setting %c", *opts);
}
opts++;
}

if (!all_opts_is_valid) {
return -EINVAL;
}

out:
config.frequency = frequency;
config.operation = operation;
spi_device = dev;

return 0;
}

SHELL_STATIC_SUBCMD_SET_CREATE(sub_spi_cmds,
SHELL_CMD_ARG(conf, &dsub_device_name,
"Configure SPI\n"
"Usage: spi conf <device> <frequency> [<settings>]\n"
"<settings> - any sequence of letters:\n"
"o - SPI_MODE_CPOL\n"
"h - SPI_MODE_CPHA\n"
"l - SPI_TRANSFER_LSB\n"
"T - SPI_FRAME_FORMAT_TI\n"
"example: spi conf spi1 1000000 ol",
cmd_spi_conf, 3, 1),
SHELL_CMD_ARG(transceive, NULL,
"Transceive data to and from an SPI device\n"
"Usage: spi transceive <TX byte 1> [<TX byte 2> ...]",
cmd_spi_transceive, 2, MAX_SPI_BYTES - 1),
SHELL_SUBCMD_SET_END);
SHELL_STATIC_SUBCMD_SET_CREATE(
sub_spi_cmds,
SHELL_CMD_ARG(transceive, &dsub_spidev,
"Transceive data to and from an SPI device\n"
"Usage: spi transceive <node> <TX byte 1> [<TX byte 2> ...]",
cmd_spi_transceive_dt, 3, MAX_SPI_BYTES - 1),
SHELL_SUBCMD_SET_END);

SHELL_CMD_REGISTER(spi, &sub_spi_cmds, "SPI commands", NULL);
Loading