[greybus-dev] Re: [PATCH v2 14/14] greybus: cpc: add CPC SDIO host driver

On Thursday 15 January 2026 16:58:07 Central European Standard Time Damien Riégel wrote:

From: Gabriel Beaulieu gabriel.beaulieu@silabs.com

This introduces a new module gb-cpc-sdio, in order to communicate with a Greybus CPC device over SDIO.

Most of the complexity stems from aggregation: packets are aggregated to minimize the number of CMD53s. In the first block, the first le32 is the number of packets in this transfer. Immediately after that are all the packet headers (CPC + Greybus). This lets the device process all the headers in a single interrupt, and prepare the ADMA descriptors for all the payloads in one go.

Payloads start at the beginning of the second block and are concatained. Their lengths must be 32-bit aligned, so the driver takes care of adding and removing padding if necessary.

Signed-off-by: Gabriel Beaulieu gabriel.beaulieu@silabs.com Signed-off-by: Damien Riégel damien.riegel@silabs.com

---

Changes in v2:

• change formatting from %lu to %zu when printing size_t's
• remove "/**" kernel-doc marker for static functions not actually using the kernel-doc format
• reduce header inclusion list
• use reverse christmas tree variable declarations consistently
• update aggregation functions to try to be more legible
• use define instead of constant value 0x0C for the address where to read the number of bytes the device wants to send
• remove padding between headers and payloads when aggregating packets

drivers/greybus/cpc/Kconfig | 12 + drivers/greybus/cpc/Makefile | 3 + drivers/greybus/cpc/sdio.c | 533 +++++++++++++++++++++++++++++++++++ 3 files changed, 548 insertions(+) create mode 100644 drivers/greybus/cpc/sdio.c

diff --git a/drivers/greybus/cpc/Kconfig b/drivers/greybus/cpc/Kconfig index ab96fedd0de..8223f56795f 100644 --- a/drivers/greybus/cpc/Kconfig +++ b/drivers/greybus/cpc/Kconfig @@ -8,3 +8,15 @@ config GREYBUS_CPC To compile this code as a module, chose M here: the module will be called gb-cpc.ko

+config GREYBUS_CPC_SDIO

• tristate "Greybus CPC over SDIO"
• depends on GREYBUS_CPC && MMC

Don't you think, this PR should also be sent to the SDIO maintainers (linux-mmc@vger.kernel.org)?

• help

•  This driver provides Greybus CPC host support for devices
  

•  connected via SDIO interface.
  

For now, I believe only some Silicon Labs device provide this interface. Maybe it would help the user to mention that.

•  To compile this driver as a module, choose M here: the module
  

•  will be called gb-cpc-sdio.
  

•  If unsure, say N.
  

[...]

diff --git a/drivers/greybus/cpc/sdio.c b/drivers/greybus/cpc/sdio.c new file mode 100644 index 00000000000..aeeb8378dea --- /dev/null +++ b/drivers/greybus/cpc/sdio.c @@ -0,0 +1,533 @@ +// SPDX-License-Identifier: GPL-2.0 +/*

• • Copyright (c) 2025, Silicon Laboratories, Inc.
• */

+#include <linux/kthread.h> +#include <linux/mmc/sdio_func.h> +#include <linux/mmc/sdio_ids.h> +#include <linux/skbuff.h> +#include <linux/workqueue.h>

+#include "cpc.h" +#include "header.h" +#include "host.h"

+#define GB_CPC_SDIO_MSG_SIZE_MAX 4096 +#define GB_CPC_SDIO_BLOCK_SIZE 256U

What is the purpose of the "U"?

+#define GB_CPC_SDIO_FIFO_ADDR 0 +#define GB_CPC_SDIO_RX_BYTES_CNT_ADDR 0x0C

It took me a bit of time to understand the meaning of these two definitions. I suggest to separate them from the other defines and use a common prefix:

#define GB_CPC_SDIO_ADDR_FIFO 0x00 #define GB_CPC_SDIO_ADDR_RX_BYTES_CNT 0x0c

+#define GB_CPC_SDIO_ALIGNMENT 4 +#define GB_CPC_SDIO_DEFAULT_AGGREGATION 1

This number seems arbitrary and it used only once. I feel it just add one layer of indirection rather than improving the readability.

+#define CPC_FRAME_HEADER_SIZE (CPC_HEADER_SIZE + GREYBUS_HEADER_SIZE) +#define GB_CPC_SDIO_MAX_AGGREGATION ((GB_CPC_SDIO_BLOCK_SIZE - sizeof(u32)) / CPC_FRAME_HEADER_SIZE)

Orphan?

[...]

+static int gb_cpc_sdio_wake_tx(struct cpc_host_device *cpc_hd) +{

• struct cpc_sdio *ctx = cpc_hd_to_cpc_sdio(cpc_hd);
• if (test_bit(CPC_SDIO_FLAG_SHUTDOWN, &ctx->flags))

• return 0;
  

• /* Use system workqueue for TX processing */

I wonder if these trivial comments are useful or rather pollute the code.

[...]

+static const struct sdio_device_id sdio_ids[] = {

•   {
  

•           SDIO_DEVICE(0x0296, 0x5347),
  

^^^^^^

You have to add an entry in include/linux/mmc/sdio_ids.h. But, it seems there is a conflict with SDIO_VENDOR_ID_MICROCHIP_WILC.

•   },
  

•   {},
  

+};

[...]