| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <473d842f-5d70-4bd9-8cb0-a4c28acd5fe4@oss.qualcomm.com>
Date: Mon, 1 Dec 2025 22:54:32 +0530
From: Praveen Talari <praveen.talari@....qualcomm.com>
To: Bjorn Andersson <andersson@...nel.org>
Cc: Andi Shyti <andi.shyti@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley
<conor+dt@...nel.org>,
Mukesh Kumar Savaliya <mukesh.savaliya@....qualcomm.com>,
Viken Dadhaniya <viken.dadhaniya@....qualcomm.com>,
Konrad Dybcio <konradybcio@...nel.org>, linux-arm-msm@...r.kernel.org,
linux-i2c@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, psodagud@...cinc.com, djaggi@...cinc.com,
quic_msavaliy@...cinc.com, quic_vtanuku@...cinc.com,
quic_arandive@...cinc.com, quic_shazhuss@...cinc.com
Subject: Re: [PATCH v1 12/12] i2c: qcom-geni: Enable I2C on SA8255p Qualcomm
platforms
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@....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
>>
Powered by blists - more mailing lists