Skip to content

drivers: Clean up NXP MCUX LPSPI driver #78789

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

Merged
nashif merged 18 commits into from
Nov 28, 2024
Merged

drivers: Clean up NXP MCUX LPSPI driver #78789

Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
21ac4a8
drivers: spi_mcux_lpspi: Organize top of file
Sep 20, 2024
56c3a7f
drivers: spi_mcux_lpspi: Remove parent_dev field
Sep 20, 2024
3993e44
drivers: spi_mcux_lpspi: Move init to end of file
Sep 20, 2024
f7733ee
drivers: spi_mcux_lpspi: Clean up init function
Sep 20, 2024
d6d2cfa
drivers: spi_mcux_lpspi: Create DMA dev helpers
Sep 20, 2024
3caf9ee
drivers: spi_mcux_lpspi: Minor RTIO cleanup
Sep 20, 2024
a383f36
drivers: spi_mcux_lpspi: Consolidate IRQ handle
Sep 20, 2024
086dd02
drivers: spi_mcux_lpspi: Group API functions
Sep 20, 2024
0503400
drivers: spi_mcux_lpspi: clean up transceive calls
Sep 20, 2024
6c3db02
drivers: spi_mcux_lpspi: Remove duplicate bufsetup
Sep 20, 2024
588f749
drivers: spi_mcux_lpspi: Remove commented code
Sep 20, 2024
ff946d3
drivers: spi_mcux_lpspi: Clean up next_packet func
Sep 20, 2024
debbf3b
drivers: spi_mcux_lpspi: Macro for master cfg flag
Sep 20, 2024
b006783
drivers: spi_mcux_lpspi: ErrLog dev in iodev_start
Sep 20, 2024
a04e110
drivers: spi_mcux_lpspi: Remove rtio prepare_start
Sep 20, 2024
ba49546
drivers: spi_mcux_lpspi: Clean up configure func
Sep 20, 2024
700ef5e
drivers: spi_mcux_lpspi: Optimize dma callback
Sep 21, 2024
bcb563b
drivers: spi_mcux_lpspi: Clean up DMA path
Sep 21, 2024
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
52 changes: 35 additions & 17 deletions drivers/spi/spi_mcux_lpspi.c
View file
Open in desktop
Copy link
Contributor

@yvanderv yvanderv Nov 20, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

c6a1daedbf91aea031a58bf9882ab508ef863cc5: please fix the typo in your git commit message - 'definitin'
4fa061e6ff0bcd89edbb3388a20322f54c18b844: suggest to change your summary line to better reflect what the commit is about
41ed57c60a8c8889620be4743804b425613f969e: make the summary line a complete sentence. Bottom of what?
fd1daae6326d5ae4b0308051fb928c4298df52d4: please clarify one liner, wrap?
b3de6553518bfde70ccb01732c6c2e0a148988b9: summary line is not clear, make it a sentence.
04f52cac07a9bff4ca3457e3dd24ad4a24ebacff: please add func or function to the summary line.

Copy link
Member Author

@decsny decsny Nov 21, 2024

Choose a reason for hiding this comment

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

4fa061e: suggest to change your summary line to better reflect what the commit is about

i only get 50 characters to work with here , I changed it but i dont know what else to say, i used the maximum number of characters in the new revision

41ed57c: make the summary line a complete sentence. Bottom of what?

there are not enough characters left to say "bottom of file" , so i put "end" of file, even though the commit message says already "bottom of file" , so im not sure why someone would be confused, now it uses the maximum number of characters

fd1daae: please clarify one liner, wrap?

there are not enough characters left to type the full word "wrappers", so i changed it to "calls", which is the maximum number of characters

b3de655: summary line is not clear, make it a sentence.

there are not enough characters for a sentence, so i changed it to "ErrLog dev in iodev_start" which is using the maximum number of characters

04f52ca: please add func or function to the summary line.

I cannot do that because the commit title already is using the maximum number of characters allowed, it should be obvious that prepare_start is a function name, especially if one reads the commit message that says "remove unneeded prepare_start function" in the first line

Original file line number Diff line number Diff line change
Expand Up @@ -272,7 +272,10 @@ static int spi_mcux_configure(const struct device *dev, const struct spi_config
}

#ifdef CONFIG_SPI_MCUX_LPSPI_DMA
static int spi_mcux_dma_rxtx_load(const struct device *dev, size_t *dma_size);
static bool lpspi_inst_has_dma(const struct spi_mcux_data *data)
{
return (data->dma_tx.dma_dev && data->dma_rx.dma_dev);
}
Copy link
Contributor

@EmilioCBen EmilioCBen Oct 15, 2024

Choose a reason for hiding this comment

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

Is the '!!' here required?

Copy link
Member Author

@decsny decsny Oct 15, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

probably not, I can remove that if I need to push again

Copy link
Contributor

@danieldegrasse danieldegrasse Oct 24, 2024

Choose a reason for hiding this comment

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

(non blocking) ping on this- looks like it could be removed

Copy link
Member Author

@decsny decsny Oct 24, 2024

Choose a reason for hiding this comment

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

it's how the spi_context code does these types of checks with returns, is why I did it


/* This function is executed in the interrupt context */
static void spi_mcux_dma_callback(const struct device *dev, void *arg, uint32_t channel, int status)
Expand Down Expand Up @@ -550,7 +553,9 @@ static int transceive_dma(const struct device *dev, const struct spi_config *spi

return ret;
}
#endif
#else
#define lpspi_inst_has_dma(arg) arg != arg
Copy link
Contributor

@mmahadevan108 mmahadevan108 Nov 21, 2024

Choose a reason for hiding this comment

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

Is the #else required? This function is only needed when CONFIG_SPI_MCUX_LPSPI_DMA is defined.

Copy link
Member Author

@decsny decsny Nov 21, 2024

Choose a reason for hiding this comment

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

yes otherwise there will be a build error

#endif /* CONFIG_SPI_MCUX_LPSPI_DMA */

#ifdef CONFIG_SPI_RTIO

Expand Down Expand Up @@ -613,7 +618,7 @@ static int spi_mcux_transceive(const struct device *dev, const struct spi_config
#ifdef CONFIG_SPI_MCUX_LPSPI_DMA
const struct spi_mcux_data *data = dev->data;

if (data->dma_rx.dma_dev && data->dma_tx.dma_dev) {
if (lpspi_inst_has_dma(data)) {
return transceive_dma(dev, spi_cfg, tx_bufs, rx_bufs, false, NULL, NULL);
}
#endif /* CONFIG_SPI_MCUX_LPSPI_DMA */
Expand All @@ -630,7 +635,7 @@ static int spi_mcux_transceive_async(const struct device *dev, const struct spi_
#ifdef CONFIG_SPI_MCUX_LPSPI_DMA
struct spi_mcux_data *data = dev->data;

if (data->dma_rx.dma_dev && data->dma_tx.dma_dev) {
if (lpspi_inst_has_dma(data)) {
spi_context_buffers_setup(&data->ctx, tx_bufs, rx_bufs, 1);
}

Expand Down Expand Up @@ -749,29 +754,42 @@ static void spi_mcux_iodev_complete(const struct device *dev, int status)

#endif

#if defined(CONFIG_SPI_MCUX_LPSPI_DMA)
static int lpspi_dma_dev_ready(const struct device *dma_dev)
{
if (!device_is_ready(dma_dev)) {
LOG_ERR("%s device is not ready", dma_dev->name);
return -ENODEV;
}

return 0;
}

static int lpspi_dma_devs_ready(struct spi_mcux_data *data)
{
return lpspi_dma_dev_ready(data->dma_tx.dma_dev) |
lpspi_dma_dev_ready(data->dma_rx.dma_dev);
}
#else
#define lpspi_dma_devs_ready(...) 0
#endif /* CONFIG_SPI_MCUX_LPSPI_DMA */

static int spi_mcux_init(const struct device *dev)
{
const struct spi_mcux_config *config = dev->config;
struct spi_mcux_data *data = dev->data;
int err;
int err = 0;

DEVICE_MMIO_NAMED_MAP(dev, reg_base, K_MEM_CACHE_NONE | K_MEM_DIRECT_MAP);

data->dev = dev;

#ifdef CONFIG_SPI_MCUX_LPSPI_DMA
if (data->dma_tx.dma_dev && data->dma_rx.dma_dev) {
if (!device_is_ready(data->dma_tx.dma_dev)) {
LOG_ERR("%s device is not ready", data->dma_tx.dma_dev->name);
return -ENODEV;
}

if (!device_is_ready(data->dma_rx.dma_dev)) {
LOG_ERR("%s device is not ready", data->dma_rx.dma_dev->name);
return -ENODEV;
}
if (IS_ENABLED(CONFIG_SPI_MCUX_LPSPI_DMA) && lpspi_inst_has_dma(data)) {
err = lpspi_dma_devs_ready(data);
}
if (err < 0) {
return err;
}
#endif /* CONFIG_SPI_MCUX_LPSPI_DMA */

err = spi_context_cs_configure_all(&data->ctx);
if (err < 0) {
Expand Down