Hi Bjorn,
Thank you for review.
On 11/26/2025 9:08 PM, Bjorn Andersson wrote:
> On Sat, Nov 22, 2025 at 10:30:16AM +0530, Praveen Talari wrote:
>> To manage GENI serial engine resources during runtime power management,
>> drivers currently need to call functions for ICC, clock, and
>> SE resource operations in both suspend and resume paths, resulting in
>> code duplication across drivers.
>>
>> The new geni_se_resources_state() helper API addresses this issue by
>> providing a streamlined method to enable or disable all resources
>> based on a boolean parameter, thereby eliminating redundancy across
>> drivers.
>>
>> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
>> ---
>> drivers/i2c/busses/i2c-qcom-geni.c | 28 +++++-----------------------
>> 1 file changed, 5 insertions(+), 23 deletions(-)
>
> Nice to see such stats, which I presume will also show up in the other
> SE drivers later as well.
Sure.
>
>>
>> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
>> index 790cbca2c22e..ea117a4667e0 100644
>> --- a/drivers/i2c/busses/i2c-qcom-geni.c
>> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
>> @@ -1166,18 +1166,15 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
>> struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
>>
>> disable_irq(gi2c->irq);
>> - ret = geni_se_resources_off(&gi2c->se);
>> +
>> + ret = geni_se_resources_state(&gi2c->se, false);
>
> As I said in the previous patch, there's no reason to "set state to
> false", it's clearer to just have an "on" and an "off" function.
Sure. Will review and update in next patch
Thanks,
Praveen
>
> Regards,
> Bjorn
>
>> if (ret) {
>> enable_irq(gi2c->irq);
>> return ret;
>> -
>> - } else {
>> - gi2c->suspended = 1;
>> }
>>
>> - clk_disable_unprepare(gi2c->core_clk);
>> -
>> - return geni_icc_disable(&gi2c->se);
>> + gi2c->suspended = 1;
>> + return ret;
>> }
>>
>> static int __maybe_unused geni_i2c_runtime_resume(struct device *dev)
>> @@ -1185,28 +1182,13 @@ static int __maybe_unused geni_i2c_runtime_resume(struct device *dev)
>> int ret;
>> struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
>>
>> - ret = geni_icc_enable(&gi2c->se);
>> + ret = geni_se_resources_state(&gi2c->se, true);
>> if (ret)
>> return ret;
>>
>> - ret = clk_prepare_enable(gi2c->core_clk);
>> - if (ret)
>> - goto out_icc_disable;
>> -
>> - ret = geni_se_resources_on(&gi2c->se);
>> - if (ret)
>> - goto out_clk_disable;
>> -
>> enable_irq(gi2c->irq);
>> gi2c->suspended = 0;
>>
>> - return 0;
>> -
>> -out_clk_disable:
>> - clk_disable_unprepare(gi2c->core_clk);
>> -out_icc_disable:
>> - geni_icc_disable(&gi2c->se);
>> -
>> return ret;
>> }
>>
>> --
>> 2.34.1
>>