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

Cédric Le Goater
2026-02-27T07:51:36Z

Hello Jithu,

On 2/27/26 03:23, Jithu Joseph wrote:
>
> 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

Could please send a patch ? I will queue it after this series.

Also, since you have tested this series, would mind sending
a Tested-by tag ? possibly a Reviewed-by too.

Thanks

C.

>
>> + 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