Re: [PATCH v7 03/22] hw/i3c: Add bus support

Jithu Joseph
2026-02-27T02:24:02Z

Another minor comment which too can be addressed later

On 2/24/2026 6:12 PM, Jamin Lin wrote:
> Adds an I3C bus and a target class.
> The bus supports:
> - I3C data transmission and reception
> - CCCs (including ENTDAA)
> - IBIs
> - legacy I2C transactions
>
> General usage of the bus is similar to I2C. Users are expected to
> initialize a bus via i3c_init_bus, and use the bus returned from the
> init function to do transactions on the bus.
>
> In order to handle IBIs, the controller provides callbacks to handle
> receiving an IBI from a target, receiving (optional) additional IBI
> bytes from a target, and handling when a target is done with its IBI.
>
> Similarly, target creation is done via i3c_target_create_simple and
> users use the provided I3CTarget to handle transactions.
> The target has functions provided that it can use to invoke an IBI and
> send additional bytes.
>
> Along with the send, recv, and event callbacks that are expected of an
> I3C target, which are similar to I2C, there is a separate callback for
> CCC handling.
> This is to help encapsulate CCC handling and keep it separate from
> target-specific read/write functionality.
>
> To avoid repition for required CCCs among I3C targets, there is some
> class-level CCC handling added. The CCC is then passed to the target in
> case it needs to handle it in some way.
>
> Signed-off-by: Joe Komlodi <komlodi@google.com>
> Reviewed-by: Patrick Venture <venture@google.com>
> Reviewed-by: Titus Rwantare <titusr@google.com>
> Reviewed-by: Jamin Lin <jamin_lin@aspeedtech.com>
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
> include/hw/i3c/i3c.h | 277 ++++++++++++++++++
> hw/i3c/core.c | 647 +++++++++++++++++++++++++++++++++++++++++++
> hw/i3c/meson.build | 1 +
> hw/i3c/trace-events | 16 ++
> 4 files changed, 941 insertions(+)
> create mode 100644 include/hw/i3c/i3c.h
> create mode 100644 hw/i3c/core.c
>

...

> +
> +int i3c_send_byte(I3CBus *bus, uint8_t data)
> +{
> + /*
> + * Ignored, the caller can determine how many were sent based on if this was
> + * ACKed/NACKed.
> + */
> + uint32_t num_sent;

num_sent is uninitialized here. Even though i3c_send_byte ignores it after the call, it gets passed by pointer into i3c_send() where it is used in the trace_i3c_send() call.
If the send callback does not write *num_sent (as is the case with mock_i3c_target_tx, refer my prev comment for patch 18/22), the trace might show garbage. Initializing it to zero might reduce the confusion

> + return i3c_send(bus, &data, 1, &num_sent);
> +}
> +
> +int i3c_send(I3CBus *bus, const uint8_t *data, uint32_t num_to_send,
> + uint32_t *num_sent)
> +{
> + I3CTargetClass *tc;
> + I3CTarget *t;
> + I3CNode *node;
> + int ret = 0;
> +
> + /* If this message is a broadcast and no CCC has been found, grab it. */
> + if (bus->broadcast && !bus->in_ccc) {
> + bus->ccc = *data;
> + bus->in_ccc = true;
> + /*
> + * We need to keep track if we're currently in ENTDAA.
> + * On any other CCC, the CCC is over on a RESTART or STOP, but ENTDAA
> + * is only over on a STOP.
> + */
> + if (bus->ccc == I3C_CCC_ENTDAA) {
> + bus->in_entdaa = true;
> + }
> + }
> +
> + QLIST_FOREACH(node, &bus->current_devs, next) {
> + t = node->target;
> + tc = I3C_TARGET_GET_CLASS(t);
> + if (bus->in_ccc) {
> + if (!tc->handle_ccc_write) {
> + ret = -1;
> + continue;
> + }
> + ret = i3c_target_handle_ccc_write(t, data, num_to_send, num_sent);
> + /* Targets should only NACK on a direct CCC. */
> + if (ret && !CCC_IS_DIRECT(bus->ccc)) {
> + ret = 0;
> + }
> + } else {
> + if (tc->send) {
> + ret = ret || tc->send(t, data, num_to_send, num_sent);
> + } else {
> + ret = -1;
> + }
> + }
> + }
> +
> + trace_i3c_send(*num_sent, num_to_send, ret == 0);

This trace might print garbage if the target send callback doesnt set it

> +
> + return ret ? -1 : 0;
> +}
> +

Thanks
Jithu