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

Bjorn Andersson
2025-11-26T15:13:48Z

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
>