drivers: spi: xec GPSPI driver update #59239

Manimaran-A · 2023-06-14T13:39:18Z

SPI driver for Microchip XEC family General Purpose SPI (GPSPI) controller. GPSPI is a byte oriented controller with hardware support of chip select assertion. The controller and driver also supports GPIO control of SPI chip select based upon the device tree configuration. The driver supports SPI asynchronous mode for byte by byte transfers (no DMA at this time).

Manimaran-A · 2023-07-05T14:42:28Z

@albertofloyd Could you help to review

drivers/spi/spi_xec_gpspi.c

tbursztyka · 2023-07-11T08:58:44Z

drivers/spi/spi_xec_gpspi.c

+	struct spi_context ctx;
+	struct spi_config scfg;
+	uint8_t configured;
+	uint8_t gpstatus;


Re-arranged the struct members.

tbursztyka · 2023-07-11T08:59:07Z

drivers/spi/spi_xec_gpspi.c

+	uint8_t gpstatus;
+#ifdef CONFIG_SPI_ASYNC
+	uint8_t isr_ctx_done;
+	size_t ctx_longest_buf_len;


and finally invert the order of these 2

tbursztyka · 2023-07-11T08:59:55Z

drivers/spi/spi_xec_gpspi.c

+static int xec_gpspi_spin_yield(int *counter, int max_count)
+{
+	*counter = *counter + 1;
+


you can remove the empty line as you are testing the same variable in the if () below

Empty space has been removed

tbursztyka · 2023-07-11T09:04:21Z

drivers/spi/spi_xec_gpspi.c

+	uint32_t lines = spi_conf->operation & SPI_LINES_MASK;
+
+	if (spi_conf->operation & (SPI_OP_MODE_SLAVE | SPI_MODE_LOOP)) {
+		LOG_ERR("Does not support SPI device or loop back");


s/device/slave

Updated the log message

tbursztyka · 2023-07-11T09:06:49Z

drivers/spi/spi_xec_gpspi.c

+	uint8_t ctrl;
+
+	/* Need to reconfigure? */
+	ret = memcmp(&data->scfg, spi_conf, sizeof(struct spi_config));


just call spi_context_configured()

Inserted the spi_context_configured api to verify already configured or not.

tbursztyka · 2023-07-11T09:08:56Z

drivers/spi/spi_xec_gpspi.c

+
+	ctx->config = spi_conf;
+
+	memcpy(&data->scfg, spi_conf, sizeof(struct spi_config));


spi_conf is not meant to change (though spi api does not strictly make it constant), since it must come from dts.
So ctx has the pointer, no need no copy the thing again.

Here is the first place value is getting assigned to ctx->config. Device info will be read from application through DTS and passed as argument to this function. So very first we need to assign ctx->config.

tbursztyka · 2023-07-11T09:09:14Z

drivers/spi/spi_xec_gpspi.c

+		cur_xfer_len = spi_context_longest_current_buf(ctx);
+
+		for (size_t i = 0; i < cur_xfer_len; i++) {
+


remove this empty line.

Space has been removed.

tbursztyka · 2023-07-11T09:13:40Z

drivers/spi/spi_xec_gpspi.c

+#ifdef CONFIG_SPI_ASYNC
+/* TODO GPSPI controller is byte oriented. DMA can only be done using
+ * the XEC legacy DMA controller (no Zephyr driver). Therefore we
+ * implement asynchronous mode using GPSPI RXBF interrupt.


Looks like you are mixing up software asynchronicity and interrupt based calls.
If you controller is able to run on interrupt mode, then BOTH non-async and async calls can run on interrupt mode.

So you should not need any xec_gpspi_xfr_sync() function after all.

Agreed, but Zephyr framework has "transceive_async" and "transceive" API. In that async call, will be return immediately and after completion of transaction application call back will be invoked.

But in sync call, will be return after completion of transaction.

So "transceive" is a synchronous call, anyway we have to wait till the completion, we used polling instead of interrupt.

@tbursztyka, do you agree? or want to implement Sync call using interrupt method?

@albertofloyd could you please suggest/give direction?

@albertofloyd can you help to provide your feedback on this?

tbursztyka · 2023-07-11T09:15:46Z

drivers/spi/spi_xec_gpspi.c

+		return ret;
+	}
+#endif
+	ret = xec_gpspi_xfr_sync(dev, spi_conf);


See above comment. Your driver does not need it as it can run on interrupt mode.
CONFIG_SPI_ASYNC is only about being able to get the cb/userdata handled relevantly, not about driver's internal logic.

github-actions · 2023-11-19T00:30:34Z

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.

github-actions · 2024-01-23T00:29:53Z

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.

SPI driver for Microchip XEC family General Purpose SPI (GPSPI) controller. GPSPI is a byte oriented controller with hardware support of chip select assertion. The controller and driver also supports GPIO control of SPI chip select based upon the device tree configuration. The driver supports SPI asynchronous mode for byte by byte transfers (no DMA at this time). Signed-off-by: Manimaran A <manimaran.a@microchip.com>

Manimaran-A · 2024-02-23T04:23:06Z

Planning to do a re-architecture of this driver, so closing this PR.

Manimaran-A requested review from galak, albertofloyd and tbursztyka as code owners June 14, 2023 13:39

zephyrbot added area: SPI SPI bus area: Devicetree Binding PR modifies or adds a Device Tree binding platform: Microchip MEC Microchip MEC Platform labels Jun 14, 2023

zephyrbot requested review from jvasanth1, teburd and VenkatKotakonda June 14, 2023 13:39

zephyrbot assigned tbursztyka Jun 14, 2023

Manimaran-A force-pushed the xec_gpspi_driver branch 2 times, most recently from e62763d to 1fa5b1b Compare June 14, 2023 14:03

tbursztyka requested changes Jul 11, 2023

View reviewed changes

Manimaran-A force-pushed the xec_gpspi_driver branch 2 times, most recently from 7f1b36f to 951ae7a Compare July 19, 2023 16:48

Manimaran-A requested a review from tbursztyka July 19, 2023 17:22

nashif assigned jvasanth1 and unassigned tbursztyka Aug 9, 2023

Manimaran-A force-pushed the xec_gpspi_driver branch from 951ae7a to ce70ef7 Compare August 29, 2023 15:50

jvasanth1 previously approved these changes Sep 19, 2023

View reviewed changes

github-actions bot added the Stale label Nov 19, 2023

Manimaran-A force-pushed the xec_gpspi_driver branch from ce70ef7 to ed43efb Compare November 22, 2023 17:26

Manimaran-A dismissed jvasanth1’s stale review via acedb68 November 22, 2023 17:48

Manimaran-A force-pushed the xec_gpspi_driver branch 2 times, most recently from acedb68 to 5c0425a Compare November 22, 2023 17:54

github-actions bot removed the Stale label Nov 23, 2023

Manimaran-A force-pushed the xec_gpspi_driver branch from 5c0425a to 1c53465 Compare November 23, 2023 10:04

github-actions bot added the Stale label Jan 23, 2024

Manimaran-A force-pushed the xec_gpspi_driver branch from 1c53465 to c720f06 Compare February 6, 2024 17:16

github-actions bot removed the Stale label Feb 7, 2024

Manimaran-A closed this Feb 23, 2024

Manimaran-A deleted the xec_gpspi_driver branch February 23, 2024 04:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: spi: xec GPSPI driver update #59239

drivers: spi: xec GPSPI driver update #59239

Manimaran-A commented Jun 14, 2023

Manimaran-A commented Jul 5, 2023

tbursztyka Jul 11, 2023

Manimaran-A Jul 19, 2023 •

edited

Loading

tbursztyka Jul 11, 2023

tbursztyka Jul 11, 2023

Manimaran-A Jul 19, 2023

tbursztyka Jul 11, 2023

Manimaran-A Jul 19, 2023

tbursztyka Jul 11, 2023

Manimaran-A Jul 19, 2023

tbursztyka Jul 11, 2023

Manimaran-A Jul 19, 2023 •

edited

Loading

tbursztyka Jul 11, 2023

Manimaran-A Jul 19, 2023

tbursztyka Jul 11, 2023

Manimaran-A Jul 19, 2023

Manimaran-A Jul 23, 2023

Manimaran-A Aug 8, 2023

jvasanth1 Aug 29, 2023

tbursztyka Jul 11, 2023

github-actions bot commented Nov 19, 2023

github-actions bot commented Jan 23, 2024

Manimaran-A commented Feb 23, 2024


		ctx->config = spi_conf;

		memcpy(&data->scfg, spi_conf, sizeof(struct spi_config));

		cur_xfer_len = spi_context_longest_current_buf(ctx);

		for (size_t i = 0; i < cur_xfer_len; i++) {

drivers: spi: xec GPSPI driver update #59239

drivers: spi: xec GPSPI driver update #59239

Conversation

Manimaran-A commented Jun 14, 2023

Manimaran-A commented Jul 5, 2023

Choose a reason for hiding this comment

Manimaran-A Jul 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Manimaran-A Jul 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Nov 19, 2023

github-actions bot commented Jan 23, 2024

Manimaran-A commented Feb 23, 2024

Manimaran-A Jul 19, 2023 •

edited

Loading

Manimaran-A Jul 19, 2023 •

edited

Loading