Re: [PATCH v2 3/4] i2c: k1: enable both functional and bus clocks

On Sat, Aug 15, 2026 at 11:40:23AM +0800, Junhui Liu wrote:
> The K1 I2C controller requires both its functional clock and APB bus
> clock to operate. The device tree provides them as "func" and "bus", but
> the driver currently acquires and enables only the first clock.
> 
> Acquire both clocks by name and enable them during probe. Use explicit
> named lookups instead of the bulk clock API to align with the K1 Linux
> driver and keep the roles of the two clocks clear if functional clock
> rate configuration is needed later.
> 
> Fixes: 271546fb8e54 ("i2c: k1: add I2C driver support")
> Signed-off-by: Junhui Liu <[email protected]>
> ---
>  drivers/i2c/k1_i2c.c | 18 ++++++++++++------
>  1 file changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/k1_i2c.c b/drivers/i2c/k1_i2c.c
> index 2c7a1e0d3775..e2e4f9e53447 100644
> --- a/drivers/i2c/k1_i2c.c
> +++ b/drivers/i2c/k1_i2c.c
> @@ -51,7 +51,6 @@ struct k1_i2c {
>  struct k1_i2c_priv {
>  	int id;
>  	void __iomem *base;
> -	struct clk clk;
>  };
>  
>  /*
> @@ -465,6 +464,7 @@ static int k1_i2c_probe(struct udevice *bus)
>  {
>  	struct k1_i2c_priv *priv = dev_get_priv(bus);
>  	struct reset_ctl reset;
> +	struct clk clk;
>  	u32 speed;
>  	int ret;
>  
> @@ -487,15 +487,21 @@ static int k1_i2c_probe(struct udevice *bus)
>  		return ret;
>  	}
>  
> -	ret = clk_get_by_index(bus, 0, &priv->clk);
> +	ret = clk_get_by_name(bus, "func", &clk);
>  	if (ret)
>  		return ret;
>  
> -	ret = clk_enable(&priv->clk);
> -	if (ret && ret != -ENOSYS && ret != -EOPNOTSUPP) {
> -		debug("%s: failed to enable clock\n", __func__);
> +	ret = clk_enable(&clk);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_get_by_name(bus, "bus", &clk);
> +	if (ret)
> +		return ret;
> +
> +	ret = clk_enable(&clk);
> +	if (ret)

I prefer to have something logged when any of these operations fail so
it's easer to debug. With dev_* or log_* APIs, they should be optimized
out and taken no extra space if they don't reach the logging level.

But anyway, we currently have many drivers silently bail out in such
cases, so this isn't a hard requirement.

Best regards,
Yao Zi

>  		return ret;
> -	}
>  
>  	priv->base = (void *)devfdt_get_addr_ptr(bus);
>  
> 
> -- 
> 2.55.0
>