linux-kernel - Re: [PATCH v1 3/4] soc: qcom: geni-se: Export function geni_se_clks

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: <45298600-beaf-438f-979a-3cb9e207a32e@linaro.org>
Date: Thu, 29 Aug 2024 11:19:14 +0100
From: Bryan O'Donoghue <bryan.odonoghue@...aro.org>
To: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>,
 konrad.dybcio@...aro.org, andersson@...nel.org, andi.shyti@...nel.org,
 linux-arm-msm@...r.kernel.org, dmaengine@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-i2c@...r.kernel.org
Cc: quic_vdadhani@...cinc.com
Subject: Re: [PATCH v1 3/4] soc: qcom: geni-se: Export function
 geni_se_clks_off()

On 29/08/2024 10:24, Mukesh Kumar Savaliya wrote:
> Currently driver provides geni_se_resources_off() function to turn
> off SE resources like clocks, pinctrl. But we don't have function to
> control clock separately, hence export function geni_se_clks_off()
> to turn off clocks separately without disturbing GPIO.
> 
> The client drivers like i2c requires this function for use case where
> i2c SE is shared between two subsystems.
> 
> Signed-off-by: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>

Suggest:

Currently the driver provides a function called 
geni_serial_resources_off() to turn off resources like clocks and 
pinctrl. We don't have a function to control clocks separately hence, 
export the function geni_se_clks_off() to turn off clocks separately 
without disturbing GPIO.

Client drivers like I2C require this function for use-cases where the 
I2C SE is shared between two subsystems.

> ---
>   drivers/soc/qcom/qcom-geni-se.c  | 4 +++-
>   include/linux/soc/qcom/geni-se.h | 3 +++
>   2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> index 2e8f24d5da80..20166c8fc919 100644
> --- a/drivers/soc/qcom/qcom-geni-se.c
> +++ b/drivers/soc/qcom/qcom-geni-se.c
> @@ -1,5 +1,6 @@
>   // SPDX-License-Identifier: GPL-2.0
>   // Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
> +// Copyright (c) 2024 Qualcomm Innovation Center, Inc. All rights reserved.
>   
>   /* Disable MMIO tracing to prevent excessive logging of unwanted MMIO traces */
>   #define __DISABLE_TRACE_MMIO__
> @@ -482,13 +483,14 @@ void geni_se_config_packing(struct geni_se *se, int bpw, int pack_words,
>   }
>   EXPORT_SYMBOL_GPL(geni_se_config_packing);
>   
> -static void geni_se_clks_off(struct geni_se *se)
> +void geni_se_clks_off(struct geni_se *se)
>   {
>   	struct geni_wrapper *wrapper = se->wrapper;
>   
>   	clk_disable_unprepare(se->clk);
>   	clk_bulk_disable_unprepare(wrapper->num_clks, wrapper->clks);
>   }
> +EXPORT_SYMBOL_GPL(geni_se_clks_off);
>

Does it make sense to have geni_se_clks_off() exported without having 
geni_se_clks_on() similarly exported ?

Two exported functions already appear to wrapper this functionality for you.

geni_se_resources_off -> gensi_se_clks_off
geni_se_resources_on -> gensi_se_clks_on

Seems like a usage violation to have geni_se_resources_on() switch the 
clocks on but then have something else directly call gensi_se_clks_off() 
without going through geni_se_resources_off();

?

---
bod