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

["Jérôme Pouiller" <jerome.pouiller@silabs.com>]

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.

> +       },
> +       {},
> +};

[...]

-- 
Jérôme Pouiller