Re: [PATCH v1 12/12] i2c: qcom-geni: Enable I2C on SA8255p Qualcomm platforms
From: Praveen Talari
Date: Mon Dec 01 2025 - 12:24:51 EST
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@xxxxxxxxxxxxxxxx>
---
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