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);
+	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)
+{
+	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

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
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.

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
>

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
>>