Re: [PATCH v4] i3c: master: dw-i3c-master: fix OD timing for first broadcast

[NG, TZE YEE]

On 31/7/2026 12:35 pm, sashiko-bot@xxxxxxxxxx wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Initialization failure on ACPI/DT platforms due to missing core clock fallback in dw_i3c_master_set_speed().
> --
> 
> --- Patch [1]: [PATCH v4] i3c: master: dw-i3c-master: fix OD timing for first broadcast ---
> commit c4596518222ed00cb6c15e2963a846694b7f5d55
> Author: Tze Yee Ng <tze.yee.ng@xxxxxxxxxx>
> 
> i3c: master: dw-i3c-master: fix OD timing for first broadcast
> 
> This commit implements the ->set_speed() callback to allow the I3C core to
> switch open-drain timings for the first broadcast address. It caches the
> normal OD register value during bus initialization and computes a separate
> od_hcnt for the slow path.
> 
>> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
>> index 3816a50a52cc5..11ae517cf2e60 100644
>> --- a/drivers/i3c/master/dw-i3c-master.c
>> +++ b/drivers/i3c/master/dw-i3c-master.c
> 
> [ ... ]
> 
>> @@ -1531,6 +1537,50 @@ static irqreturn_t dw_i3c_master_irq_handler(int irq, void *dev_id)
>>   	return IRQ_HANDLED;
>>   }
>>   
>> +static int dw_i3c_master_set_speed(struct i3c_master_controller *m,
>> +				   enum i3c_open_drain_speed speed)
>> +{
>> +	struct dw_i3c_master *master = to_dw_i3c_master(m);
>> +	unsigned long core_rate;
>> +	u32 scl_timing, od_hcnt;
>> +	u8 lcnt;
>> +
>> +	PM_RUNTIME_ACQUIRE_AUTOSUSPEND(master->dev, pm);
>> +	if (PM_RUNTIME_ACQUIRE_ERR(&pm))
>> +		return -ENXIO;
>> +
>> +	switch (speed) {
>> +	case I3C_OPEN_DRAIN_SLOW_SPEED:
>> +		core_rate = clk_get_rate(master->core_clk);
>> +		if (!core_rate)
>> +			return -EINVAL;
> 
> [Severity: High]
> Will this cause platforms without a core clock provider to fail here?
> 
> The rest of the driver uses dw_i3c_master_get_core_rate() to fall back to
> the "clock-frequency" device property if master->core_clk is NULL (such as
> on ACPI/DT platforms). If clk_get_rate() is called directly here on those
> platforms, it appears core_rate will be 0 and the callback will abort with
> -EINVAL.
> 

Agreed. I will switch set_speed() to use dw_i3c_master_get_core_rate() 
like the rest of the driver.

Thanks,
Tze Yee

>> +
>> +		lcnt = SCL_I3C_TIMING_LCNT(master->i3c_od_timing_normal);
>> +		od_hcnt = DIV_ROUND_UP_ULL((u64)I3C_BUS_THIGH_INIT_OD_MIN_NS *
>> +					  core_rate, NSEC_PER_SEC) - 1;
> 
> [ ... ]
>