Re: [PATCH v1 12/12] i2c: qcom-geni: Enable I2C on SA8255p Qualcomm platforms

[Praveen Talari <praveen.talari@oss.qualcomm.com>]

Hi Bjorn,

On 11/26/2025 9:22 PM, Bjorn Andersson wrote:
> On Sat, Nov 22, 2025 at 10:30:18AM +0530, Praveen Talari wrote:
>> The Qualcomm automotive SA8255p SoC relies on firmware to configure
>> platform resources, including clocks, interconnects and TLMM.
>> The driver requests resources operations over SCMI using power
>> and performance protocols.
>>
>> The SCMI power protocol enables or disables resources like clocks,
>> interconnect paths, and TLMM (GPIOs) using runtime PM framework APIs,
>> such as resume/suspend, to control power states(on/off).
>>
>> The SCMI performance protocol manages I2C frequency, with each
>> frequency rate represented by a performance level. The driver uses
>> geni_se_set_perf_opp() API to request the desired frequency rate..
>>
>> As part of geni_se_set_perf_opp(), the OPP for the requested frequency
>> is obtained using dev_pm_opp_find_freq_floor() and the performance
>> level is set using dev_pm_opp_set_opp().
>>
>> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
>> ---
>>   drivers/i2c/busses/i2c-qcom-geni.c | 46 +++++++++++++++++++++++-------
>>   1 file changed, 35 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
>> index a0f68fdd4078..78154879f02d 100644
>> --- a/drivers/i2c/busses/i2c-qcom-geni.c
>> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
>> @@ -82,6 +82,9 @@ struct geni_i2c_desc {
>>   	char *icc_ddr;
>>   	bool no_dma_support;
>>   	unsigned int tx_fifo_depth;
>> +	int (*resources_init)(struct geni_se *se);
>> +	int (*set_rate)(struct geni_se *se, unsigned long freq);
>> +	int (*power_state)(struct geni_se *se, bool state);
> 
> You have isolated this quite nicely now, so I'd prefer 3 (four to keep
> power on/off separate) if statements, over these function pointers, at
> this point.

Thank you for the feedback. I understand the preference for if 
statements, but function pointers offer better scalability here:

- Qualcomm has various power management schemes (Linux-driven vs 
firmware-assisted) across SoCs with more variants coming.
- If statements would require modifying the core driver logic for each 
new SoC variant, while function pointers isolate hardware-specific 
behavior to dedicated implementations.
- New SoC enablement becomes a matter of adding new function 
implementations rather than touching core logic.

Thanks,
Praveen
> 
> This saves the future reader from having to remember the combination of
> function pointer targets in the various cases - and allow things like
> "jump to definition" in your editor to still work.
> 
>>   };
>>   
>>   #define QCOM_I2C_MIN_NUM_OF_MSGS_MULTI_DESC	2
>> @@ -203,8 +206,9 @@ static int geni_i2c_clk_map_idx(struct geni_i2c_dev *gi2c)
>>   	return -EINVAL;
>>   }
>>   
>> -static void qcom_geni_i2c_conf(struct geni_i2c_dev *gi2c)
>> +static int qcom_geni_i2c_conf(struct geni_se *se, unsigned long freq)
> 
> This sounds like a qcom_geni_i2c_set_rate() now that it takes a
> frequency argument.
Yes because of function pointer compatible

int (*set_rate)(struct geni_se *se, unsigned long freq);

Thanks,
Praveen
> 
> Regards,
> Bjorn
> 
>>   {
>> +	struct geni_i2c_dev *gi2c = dev_get_drvdata(se->dev);
>>   	const struct geni_i2c_clk_fld *itr = gi2c->clk_fld;
>>   	u32 val;
>>   
>> @@ -217,6 +221,7 @@ static void qcom_geni_i2c_conf(struct geni_i2c_dev *gi2c)
>>   	val |= itr->t_low_cnt << LOW_COUNTER_SHFT;
>>   	val |= itr->t_cycle_cnt;
>>   	writel_relaxed(val, gi2c->se.base + SE_I2C_SCL_COUNTERS);
>> +	return 0;
>>   }
>>   
>>   static void geni_i2c_err_misc(struct geni_i2c_dev *gi2c)
>> @@ -908,7 +913,9 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
>>   		return ret;
>>   	}
>>   
>> -	qcom_geni_i2c_conf(gi2c);
>> +	ret = gi2c->dev_data->set_rate(&gi2c->se, gi2c->clk_freq_out);
>> +	if (ret)
>> +		return ret;
>>   
>>   	if (gi2c->gpi_mode)
>>   		ret = geni_i2c_gpi_xfer(gi2c, msgs, num);
>> @@ -1041,8 +1048,9 @@ static int geni_i2c_init(struct geni_i2c_dev *gi2c)
>>   	return ret;
>>   }
>>   
>> -static int geni_i2c_resources_init(struct geni_i2c_dev *gi2c)
>> +static int geni_i2c_resources_init(struct geni_se *se)
>>   {
>> +	struct geni_i2c_dev *gi2c = dev_get_drvdata(se->dev);
>>   	int ret;
>>   
>>   	ret = geni_se_resources_init(&gi2c->se);
>> @@ -1095,7 +1103,7 @@ static int geni_i2c_probe(struct platform_device *pdev)
>>   	spin_lock_init(&gi2c->lock);
>>   	platform_set_drvdata(pdev, gi2c);
>>   
>> -	ret = geni_i2c_resources_init(gi2c);
>> +	ret = gi2c->dev_data->resources_init(&gi2c->se);
>>   	if (ret)
>>   		return ret;
>>   
>> @@ -1165,10 +1173,12 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
>>   
>>   	disable_irq(gi2c->irq);
>>   
>> -	ret = geni_se_resources_state(&gi2c->se, false);
>> -	if (ret) {
>> -		enable_irq(gi2c->irq);
>> -		return ret;
>> +	if (gi2c->dev_data->power_state) {
>> +		ret = gi2c->dev_data->power_state(&gi2c->se, false);
>> +		if (ret) {
>> +			enable_irq(gi2c->irq);
>> +			return ret;
>> +		}
>>   	}
>>   
>>   	gi2c->suspended = 1;
>> @@ -1180,9 +1190,11 @@ static int __maybe_unused geni_i2c_runtime_resume(struct device *dev)
>>   	int ret;
>>   	struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
>>   
>> -	ret = geni_se_resources_state(&gi2c->se, true);
>> -	if (ret)
>> -		return ret;
>> +	if (gi2c->dev_data->power_state) {
>> +		ret = gi2c->dev_data->power_state(&gi2c->se, true);
>> +		if (ret)
>> +			return ret;
>> +	}
>>   
>>   	enable_irq(gi2c->irq);
>>   	gi2c->suspended = 0;
>> @@ -1221,6 +1233,9 @@ static const struct dev_pm_ops geni_i2c_pm_ops = {
>>   
>>   static const struct geni_i2c_desc geni_i2c = {
>>   	.icc_ddr = "qup-memory",
>> +	.resources_init = geni_i2c_resources_init,
>> +	.set_rate = qcom_geni_i2c_conf,
>> +	.power_state = geni_se_resources_state,
>>   };
>>   
>>   static const struct geni_i2c_desc i2c_master_hub = {
>> @@ -1228,11 +1243,20 @@ static const struct geni_i2c_desc i2c_master_hub = {
>>   	.icc_ddr = NULL,
>>   	.no_dma_support = true,
>>   	.tx_fifo_depth = 16,
>> +	.resources_init = geni_i2c_resources_init,
>> +	.set_rate = qcom_geni_i2c_conf,
>> +	.power_state = geni_se_resources_state,
>> +};
>> +
>> +static const struct geni_i2c_desc sa8255p_geni_i2c = {
>> +	.resources_init = geni_se_domain_attach,
>> +	.set_rate = geni_se_set_perf_opp,
>>   };
>>   
>>   static const struct of_device_id geni_i2c_dt_match[] = {
>>   	{ .compatible = "qcom,geni-i2c", .data = &geni_i2c },
>>   	{ .compatible = "qcom,geni-i2c-master-hub", .data = &i2c_master_hub },
>> +	{ .compatible = "qcom,sa8255p-geni-i2c", .data = &sa8255p_geni_i2c },
>>   	{}
>>   };
>>   MODULE_DEVICE_TABLE(of, geni_i2c_dt_match);
>> -- 
>> 2.34.1
>>