Re: [PATCH v1 04/12] soc: qcom: geni-se: Add geni_se_resource_state() helper

Praveen Talari
2025-11-28T04:42:29Z

H Bjorn

On 11/26/2025 8:49 PM, Bjorn Andersson wrote:
> On Sat, Nov 22, 2025 at 10:30:10AM +0530, Praveen Talari wrote:
>> The GENI SE protocol drivers (I2C, SPI, UART) implement similar resource
>> activation/deactivation sequences independently, leading to code
>> duplication.
>>
>> Introduce geni_se_resource_state() to control power state of GENI SE
>> resources. This function provides a unified interface that calls either
>> geni_se_resources_activate() to power on resources or
>> geni_se_resources_deactivate() to power off resources based on the
>> power_on parameter.
>>
>> The activate function enables ICC, clocks, and TLMM with proper error
>> handling and cleanup paths. The deactivate function disables resources
>> in reverse order including OPP rate reset, clocks, ICC and TLMM.
>>
>> Signed-off-by: Praveen Talari <praveen.talari@oss.qualcomm.com>
>> ---
>> drivers/soc/qcom/qcom-geni-se.c | 61 ++++++++++++++++++++++++++++++++
>> include/linux/soc/qcom/geni-se.h | 2 ++
>> 2 files changed, 63 insertions(+)
>>
>> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
>> index 726b77650007..7aee7fd2e240 100644
>> --- a/drivers/soc/qcom/qcom-geni-se.c
>> +++ b/drivers/soc/qcom/qcom-geni-se.c
>> @@ -1013,6 +1013,67 @@ int geni_icc_disable(struct geni_se *se)
>> }
>> EXPORT_SYMBOL_GPL(geni_icc_disable);
>>
>> +static int geni_se_resources_deactivate(struct geni_se *se)
>> +{
>> + int ret;
>> +
>> + if (se->has_opp)
>> + dev_pm_opp_set_rate(se->dev, 0);
>> +
>> + ret = geni_se_resources_off(se);
>
> Why do we end this series with two different APIs for turning (on/) off

Currently, we have resources_off() which only manages clocks and
pinctrl. I’m leveraging that in the new API.

If you agree, I can migrate the logic from resources_off() into the new
API and remove resources_off() once support for all protocols is
implemented.

Code snippet:

static int geni_se_resources_deactivate(struct geni_se *se)
{
int ret;

if (has_acpi_companion(se->dev))
return 0;

if (se->has_opp)
dev_pm_opp_set_rate(se->dev, 0);

ret = pinctrl_pm_select_sleep_state(se->dev);
if (ret)
return ret;

geni_se_clks_off(se);

if (se->core_clk)
clk_disable_unprepare(se->core_clk);

return geni_icc_disable(se);
}

static int geni_se_resources_activate(struct geni_se *se)
{
int ret;

if (has_acpi_companion(se->dev))
return 0;

ret = geni_icc_enable(se);
if (ret)
return ret;

if (se->core_clk) {
ret = clk_prepare_enable(se->core_clk);
if (ret)
goto out_icc_disable;
}

ret = geni_se_clks_on(se);
if (ret)
goto out_clk_disable;

ret = pinctrl_pm_select_default_state(se->dev);
if (ret) {
geni_se_clks_off(se);
goto out_clk_disable;
}

return ret;

out_clk_disable:
if (se->core_clk)
clk_disable_unprepare(se->core_clk);
out_icc_disable:
geni_icc_disable(se);
return ret;
}

> the GENI resources? Can't there be a single geni_se_resources_"off"()?
>
>> + if (ret)
>> + return ret;
>> +
>> + if (se->core_clk)
>> + clk_disable_unprepare(se->core_clk);
>> +
>> + return geni_icc_disable(se);
>> +}
>> +
>> +static int geni_se_resources_activate(struct geni_se *se)
>> +{
>> + int ret;
>> +
>> + ret = geni_icc_enable(se);
>> + if (ret)
>> + return ret;
>> +
>> + if (se->core_clk) {
>> + ret = clk_prepare_enable(se->core_clk);
>> + if (ret)
>> + goto out_icc_disable;
>> + }
>> +
>> + ret = geni_se_resources_on(se);
>> + if (ret)
>> + goto out_clk_disable;
>> +
>> + return 0;
>> +
>> +out_clk_disable:
>> + if (se->core_clk)
>> + clk_disable_unprepare(se->core_clk);
>> +out_icc_disable:
>> + geni_icc_disable(se);
>> + return ret;
>> +}

>> +
>> +/**
>> + * geni_se_resources_state() - Control power state of GENI SE resources
>> + * @se: Pointer to the geni_se structure
>> + * @power_on: Boolean flag for desired power state (true = on, false = off)
>> + *
>> + * Controls GENI SE resource power state by calling activate or deactivate
>> + * functions based on the power_on parameter.
>> + *
>> + * Return: 0 on success, negative error code on failure
>> + */
>> +int geni_se_resources_state(struct geni_se *se, bool power_on)
>
> It seems the purpose of this "helper function" is to allow replacing
> geni_se_resource_on() with geni_se_resources_state(true) and
> geni_se_resource_off() with geni_se_resources_state(false) in patch 10.
>
>
> Naming a function "on", "activate", or "enable" provides a clear
> indication of what will happen when you call the function. Calling a
> function to "set state to true" is not as clear.
>
> Further, the code paths that needs to have resources turned on should be
> separate from those who signal that those resources can be turned off.
> So there should not be any gain from this function, unless the same
> obfuscation happens further up the stack.
>
> Just call the activate/deactivate in the respective code path.

Thank you for the inputs.
Sure, will review and update next patch.

Thanks,
Praveen Talari
>
> Regards,
> Bjorn
>
>> +{
>> + return power_on ? geni_se_resources_activate(se) : geni_se_resources_deactivate(se);
>> +}
>> +EXPORT_SYMBOL_GPL(geni_se_resources_state);
>> +
>> /**
>> * geni_se_resources_init() - Initialize resources for a GENI SE device.
>> * @se: Pointer to the geni_se structure representing the GENI SE device.
>> diff --git a/include/linux/soc/qcom/geni-se.h b/include/linux/soc/qcom/geni-se.h
>> index c182dd0f0bde..d1ca13a4e54c 100644
>> --- a/include/linux/soc/qcom/geni-se.h
>> +++ b/include/linux/soc/qcom/geni-se.h
>> @@ -541,6 +541,8 @@ int geni_icc_disable(struct geni_se *se);
>>
>> int geni_se_resources_init(struct geni_se *se);
>>
>> +int geni_se_resources_state(struct geni_se *se, bool power_on);
>> +
>> int geni_load_se_firmware(struct geni_se *se, enum geni_se_protocol_type protocol);
>> #endif
>> #endif
>> --
>> 2.34.1
>>