Re: [PATCH v3 8/8] i2c: qcom-geni: Use geni_se_set_rate() for setting source clock frequency

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] i2c: qcom-geni: severe overhead from per-transfer ICC/OPP voting
- [Medium] i2c: qcom-geni: DMA channel leak on clk_map error path
- [Low] i2c: dead code in struct geni_i2c_desc after removing set_rate usage
--

commit 180f114c4a84db9299622afd7b4abba3adcdb2b5
Author: Praveen Talari <praveen.talari@oss.qualcomm.com>
Subject: i2c: qcom-geni: Use geni_se_set_rate() for setting source clock frequency

This patch switches the GENI I2C driver to use geni_se_set_rate() instead of
platform-specific callbacks for source clock programming. It performs the
source clock configuration from qcom_geni_i2c_conf(), abstracting the
underlying resource management and unifying the code path across platforms.

> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index dbe344719b86c..4879170a12576 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c

[ ... ]

> @@ -1042,7 +1041,7 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
>  		return ret;
>  	}
>  
> -	ret = gi2c->dev_data->set_rate(&gi2c->se, gi2c->clk_freq_out);
> +	ret = qcom_geni_i2c_conf(&gi2c->se);
>  	if (ret)
>  		goto out_pm;
>  

[Severity: High]
Is there a significant performance regression from moving the interconnect
bandwidth vote and OPP setting into qcom_geni_i2c_conf()?

Because qcom_geni_i2c_conf() is now called unconditionally on every I2C
transaction via geni_i2c_xfer(), the interconnect and OPP frameworks are
invoked per transfer. This acquires global mutexes (like icc_bw_lock) and
recalculates hardware constraints, which could introduce massive overhead on
this hot path.

[ ... ]

> @@ -1345,7 +1344,6 @@ static const struct dev_pm_ops geni_i2c_pm_ops = {
>  
>  static const struct geni_i2c_desc geni_i2c = {
>  	.resources_init = geni_se_resources_init,
> -	.set_rate = qcom_geni_i2c_conf,
>  	.power_on = geni_se_resources_activate,
>  	.power_off = geni_se_resources_deactivate,
>  };
> @@ -1354,14 +1352,12 @@ static const struct geni_i2c_desc i2c_master_hub = {
>  	.no_dma_support = true,
>  	.tx_fifo_depth = 16,
>  	.resources_init = geni_se_resources_init,
> -	.set_rate = qcom_geni_i2c_conf,
>  	.power_on = geni_se_resources_activate,
>  	.power_off = geni_se_resources_deactivate,
>  };
>  
>  static const struct geni_i2c_desc sa8255p_geni_i2c = {
>  	.resources_init = geni_se_domain_attach,
> -	.set_rate = geni_se_set_perf_opp,
>  };

[Severity: Low]
Is the set_rate function pointer now dead code?

The patch completely removes the initialization of .set_rate from all
geni_i2c_desc instances, but it leaves the function pointer inside
struct geni_i2c_desc:

drivers/i2c/busses/i2c-qcom-geni.c:geni_i2c_desc {
    ...
    int (*resources_init)(struct geni_se *se);
    int (*set_rate)(struct geni_se *se, unsigned long freq);
    int (*power_on)(struct geni_se *se);
    ...
}


I also noticed a resource leak in geni_i2c_probe() that appears to have been
introduced earlier in this series when geni_i2c_clk_map_idx() was moved:

drivers/i2c/busses/i2c-qcom-geni.c:geni_i2c_probe() {
    ...
    ret = geni_i2c_init(gi2c);
    if (ret < 0)
        return ret;

    ret = geni_i2c_clk_map_idx(gi2c);
    if (ret)
        return dev_err_probe(gi2c->se.dev, ret, "Invalid clk frequency %d Hz\n",
                     gi2c->clk_freq_out);
    ...
}

[Severity: Medium]
Are the GPI DMA channels leaked on this error path?

This problem wasn't introduced by this patch, but a previous commit in this
series moved geni_i2c_clk_map_idx() into geni_i2c_probe() after
geni_i2c_init(). 

geni_i2c_init() allocates GPI DMA channels via setup_gpi_dma() if FIFO is
disabled. If geni_i2c_clk_map_idx() then fails, the probe function returns
immediately without calling release_gpi_dma(), causing a leak of the
allocated channels.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-derive_clk_perf_tbl_from_perf_domain_opp_table-v3-0-5da1096243c9@oss.qualcomm.com?part=8