Re: [PATCH 7/8] firmware: arm_scmi: transport: Add ACPI PCC transport

[Jonathan Cameron <jonathan.cameron@huawei.com>]

On Fri, 17 Oct 2025 14:23:50 +0100
Sudeep Holla <sudeep.holla@arm.com> wrote:

> Introduce a new SCMI transport that uses ACPI PCCT (PCC) subspaces via
> the Linux PCC mailbox layer. The driver parses ACPI _DSD data to map
> protocols to PCC subspace UIDs, supports shared TX/RX channels, and
> optionally sets up a P2A channel for notifications.
> 
> Key points:
> - new CONFIG_ARM_SCMI_TRANSPORT_PCC option
> - integration with SCMI core via scmi_desc and transport ops
> - response/notification fetch from PCC shared memory header/payload
> - ACPI device matching and registration via the ACPI transport macro
> 
> This enables SCMI to be exercised over PCC on ACPI platforms.
> 
> Signed-off-by: Sudeep Holla <sudeep.holla@arm.com>
Hi Sudeep,

Just a very quick look in what is definitely a drive by style review
A few things below.

> diff --git a/drivers/firmware/arm_scmi/transports/pcc.c b/drivers/firmware/arm_scmi/transports/pcc.c
> new file mode 100644
> index 000000000000..39ef83e2dfd4
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/transports/pcc.c
> @@ -0,0 +1,390 @@

> +
> +/**
> + * struct scmi_pcc - Structure representing a SCMI mailbox transport
> + *
> + * @cl: Mailbox Client
> + * @pchan: Transmit/Receive PCC/mailbox channel
> + * @cinfo: SCMI channel info
> + * @shmem: Transmit/Receive shared memory area
run kernel-doc over the file (shmem doesn't exist).

> + */
> +struct scmi_pcc {
> +	struct mbox_client cl;
> +	struct pcc_mbox_chan *pchan;
> +	struct scmi_chan_info *cinfo;
> +};

> +
> +static int acpi_scmi_dsd_parse_protocol_subpackage(const union acpi_object *obj,
> +						   int prot_id)
> +{
> +	u32 uid;
> +	int idx, ret = 0;
> +	struct pcc_transport *p;
> +	unsigned int pkg_cnt = obj->package.count;
> +
> +	if (pkg_cnt > 2) {
> +		pr_warn("Only 2 channels: one Tx and one Rx needed\n");
> +		return -EINVAL;
> +	}
> +
> +	for (idx = 0; idx < pkg_cnt; idx++) {
> +		union acpi_object *pack = &obj->package.elements[idx];
> +
> +		/* Flags(pack->package.elements[1]) must be always 0 for now */
> +		uid = pack->package.elements[0].integer.value;
> +		hash_for_each_possible(pcc_id_hash, p, hnode, uid) {
> +			if (p->flags & SCMI_TRANSPORT_SHARED_CHANNEL) {
> +				pr_info("Invalid! %d channel is shared\n",
> +					p->pcc_ss_id);
> +				ret = -EINVAL;

This breaks out of the hash_for_each_... but the outer loop might continue
and it's just possible pass.  however we leave ret set.  Why not bail out
on first error?  E.g. return -EINVAL; here.

> +				break;
> +			}
> +			p->protocol_id = prot_id;
> +		}
> +	}
> +
> +	return ret;
> +}

> +#ifdef CONFIG_ACPI

This is the bit I'd avoid by not use ACPI_PTR() in the earlier patch.

> +static const struct acpi_device_id scmi_acpi_ids[] = {
> +	{"ARML0001", 0},
> +	{}
> +};
> +MODULE_DEVICE_TABLE(acpi, scmi_acpi_ids);
> +#endif
> +
> +DEFINE_SCMI_ACPI_TRANSPORT_DRIVER(scmi_pcc, scmi_pcc_driver,
> +				  scmi_pcc_desc, scmi_acpi_ids, core);
> +module_platform_driver(scmi_pcc_driver);
> +
> +MODULE_AUTHOR("Sudeep Holla <sudeep.holla@arm.com>");
> +MODULE_DESCRIPTION("SCMI ACPI PCC Transport driver");
> +MODULE_LICENSE("GPL");
>