linux-kernel - Re: [PATCH v1 04/12] soc: qcom: geni-se: Add geni_se_resource

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <bskhxahakxpc74rdoz54eqlplb4obaoleouh4pn6qdy6yjmggw@fojwzct2haxa>
Date: Wed, 26 Nov 2025 09:19:18 -0600
From: Bjorn Andersson <andersson@...nel.org>
To: Praveen Talari <praveen.talari@....qualcomm.com>
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 04/12] soc: qcom: geni-se: Add
 geni_se_resource_state() helper

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