diff mbox series

[v2,1/2] ARM: msm8960: Rename cxo_board to cxo-board and add alias

Message ID 20220809000300.6384-1-guptarud@gmail.com (mailing list archive)
State Changes Requested
Headers show
Series [v2,1/2] ARM: msm8960: Rename cxo_board to cxo-board and add alias | expand

Commit Message

Rudraksha Gupta Aug. 9, 2022, 12:02 a.m. UTC
This patch renames cxo_board to be up to date with the current naming
style. It also adds an alias.

Signed-off-by: Rudraksha Gupta <guptarud@gmail.com>
---
v2:
 - Group the correct changes together

 arch/arm/boot/dts/qcom-msm8960.dtsi | 2 +-
 drivers/clk/qcom/gcc-msm8960.c      | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Krzysztof Kozlowski Aug. 9, 2022, 5:32 a.m. UTC | #1
On 09/08/2022 03:02, Rudraksha Gupta wrote:
> This patch renames cxo_board to be up to date with the current naming
> style. It also adds an alias.

Same comment as v1.

Additionally you do not explain why you are doing it.

> 
> Signed-off-by: Rudraksha Gupta <guptarud@gmail.com>
> ---
> v2:
>  - Group the correct changes together

Don't link new patchsets to some other threads.
> 
>  arch/arm/boot/dts/qcom-msm8960.dtsi | 2 +-
>  drivers/clk/qcom/gcc-msm8960.c      | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/qcom-msm8960.dtsi b/arch/arm/boot/dts/qcom-msm8960.dtsi
> index 0e099aa7c889..2ed969785b78 100644
> --- a/arch/arm/boot/dts/qcom-msm8960.dtsi
> +++ b/arch/arm/boot/dts/qcom-msm8960.dtsi
> @@ -58,7 +58,7 @@ cpu-pmu {
>  	};
>  
>  	clocks {
> -		cxo_board {
> +		cxo_board: cxo-board {
>  			compatible = "fixed-clock";
>  			#clock-cells = <0>;
>  			clock-frequency = <19200000>;
> diff --git a/drivers/clk/qcom/gcc-msm8960.c b/drivers/clk/qcom/gcc-msm8960.c
> index 051745ef99c8..56ce05a846dd 100644
> --- a/drivers/clk/qcom/gcc-msm8960.c
> +++ b/drivers/clk/qcom/gcc-msm8960.c
> @@ -3624,7 +3624,7 @@ static int gcc_msm8960_probe(struct platform_device *pdev)
>  	if (!match)
>  		return -EINVAL;
>  
> -	ret = qcom_cc_register_board_clk(dev, "cxo_board", "cxo", 19200000);
> +	ret = qcom_cc_register_board_clk(dev, "cxo-board", "cxo", 19200000);


My comment from v1 still applies - this does not match subsystem.
Additionally DTS change and driver change *cannot* go together.
Basically your commit is non-bisectable which is indication it is not
correct approach.

Best regards,
Krzysztof
Bjorn Andersson Aug. 10, 2022, 7:53 p.m. UTC | #2
On Mon 08 Aug 17:02 PDT 2022, Rudraksha Gupta wrote:

> This patch renames cxo_board to be up to date with the current naming
> style. It also adds an alias.
> 
> Signed-off-by: Rudraksha Gupta <guptarud@gmail.com>
> ---
> v2:
>  - Group the correct changes together
> 
>  arch/arm/boot/dts/qcom-msm8960.dtsi | 2 +-
>  drivers/clk/qcom/gcc-msm8960.c      | 2 +-

Clock and dts patches goes through two different paths towards mainline,
so they should be separated.

>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/boot/dts/qcom-msm8960.dtsi b/arch/arm/boot/dts/qcom-msm8960.dtsi
> index 0e099aa7c889..2ed969785b78 100644
> --- a/arch/arm/boot/dts/qcom-msm8960.dtsi
> +++ b/arch/arm/boot/dts/qcom-msm8960.dtsi
> @@ -58,7 +58,7 @@ cpu-pmu {
>  	};
>  
>  	clocks {
> -		cxo_board {
> +		cxo_board: cxo-board {
>  			compatible = "fixed-clock";
>  			#clock-cells = <0>;
>  			clock-frequency = <19200000>;
> diff --git a/drivers/clk/qcom/gcc-msm8960.c b/drivers/clk/qcom/gcc-msm8960.c
> index 051745ef99c8..56ce05a846dd 100644
> --- a/drivers/clk/qcom/gcc-msm8960.c
> +++ b/drivers/clk/qcom/gcc-msm8960.c
> @@ -3624,7 +3624,7 @@ static int gcc_msm8960_probe(struct platform_device *pdev)
>  	if (!match)
>  		return -EINVAL;
>  
> -	ret = qcom_cc_register_board_clk(dev, "cxo_board", "cxo", 19200000);
> +	ret = qcom_cc_register_board_clk(dev, "cxo-board", "cxo", 19200000);

This breaks compatibility with existing DTB files.

What you probably want is to make sure that any clocks with parent name
of "cxo", should have a .fw_name = "cxo", then you can make a
phandle-based reference in DT and these global names doesn't matter (and
in the end we can remove this board_clk from the driver).

Regards,
Bjorn

>  	if (ret)
>  		return ret;
>  
> -- 
> 2.25.1
>
Rudraksha Gupta Aug. 11, 2022, 2:51 a.m. UTC | #3
> Clock and dts patches goes through two different paths towards mainline,

 > so they should be separated.

Gotcha, thanks. I will do that.


 > This breaks compatibility with existing DTB files.

 > What you probably want is to make sure that any clocks with parent name

 > of "cxo", should have a .fw_name = "cxo", then you can make a

 > phandle-based reference in DT and these global names doesn't matter (and

 > in the end we can remove this board_clk from the driver).

Ah, I see. If I understand correctly, it should be something like this, 
right?
https://github.com/torvalds/linux/blob/master/drivers/clk/qcom/gcc-msm8996.c#L169-L172
Bjorn Andersson Aug. 29, 2022, 9:54 p.m. UTC | #4
On Wed, Aug 10, 2022 at 10:51:51PM -0400, Rudraksha Gupta wrote:
> > Clock and dts patches goes through two different paths towards mainline,
> 
> > so they should be separated.
> 
> Gotcha, thanks. I will do that.
> 
> 
> > This breaks compatibility with existing DTB files.
> 
> > What you probably want is to make sure that any clocks with parent name
> 
> > of "cxo", should have a .fw_name = "cxo", then you can make a
> 
> > phandle-based reference in DT and these global names doesn't matter (and
> 
> > in the end we can remove this board_clk from the driver).
> 
> Ah, I see. If I understand correctly, it should be something like this,
> right?
> https://github.com/torvalds/linux/blob/master/drivers/clk/qcom/gcc-msm8996.c#L169-L172

Correct, this will try to find the clock by clock-names of "cxo" and if
not found fall back to do a lookup globally just on the name "xo_board".

So it seems making .name "cxo_board" should handle both cases nicely.

Regards,
Bjorn
Rudraksha Gupta Feb. 21, 2023, 6:40 a.m. UTC | #5
Hello,


So I'm trying to add a cxo-board node to my dts, however the current 
implementation seems like it wants cxo_board. It was recommended a while 
ago that I refactor gcc-msm8960.c to be more like 
https://github.com/torvalds/linux/blob/master/drivers/clk/qcom/gcc-msm8996.c#L36 
. However, I have a couple of questions:

- The xo struct that I listed above is listed in another struct 
https://github.com/torvalds/linux/blob/master/drivers/clk/qcom/gcc-msm8996.c#L3408 
which is listed in the SoC desc struct 
https://github.com/torvalds/linux/blob/master/drivers/clk/qcom/gcc-msm8996.c#L3818 
. My question is that even though gcc-msm8960.c has an msm8960/apq8064 
desc struct, it doesn't have anything like gcc_msm8996_hws. How would I 
know what goes in a hypothetical gcc_(msm8960/apq8064)_hws struct? I'm 
assuming that all I need in the hw struct is the pxo and cxo listed here 
https://github.com/torvalds/linux/blob/master/drivers/clk/qcom/gcc-msm8960.c#L3727 
however I'm not 100% sure how to verify this. Would anything else go 
into a hypothetical gcc_(msm8960/apq8064)_hws struct?

- Is there documentation on how the gcc-<soc> files work? I'm still 
quite new to contributing to the Linux kernel and would like to learn 
more about the modern way to format these files and to learn more about 
how they work in general


Thanks,

Rudraksha
diff mbox series

Patch

diff --git a/arch/arm/boot/dts/qcom-msm8960.dtsi b/arch/arm/boot/dts/qcom-msm8960.dtsi
index 0e099aa7c889..2ed969785b78 100644
--- a/arch/arm/boot/dts/qcom-msm8960.dtsi
+++ b/arch/arm/boot/dts/qcom-msm8960.dtsi
@@ -58,7 +58,7 @@  cpu-pmu {
 	};
 
 	clocks {
-		cxo_board {
+		cxo_board: cxo-board {
 			compatible = "fixed-clock";
 			#clock-cells = <0>;
 			clock-frequency = <19200000>;
diff --git a/drivers/clk/qcom/gcc-msm8960.c b/drivers/clk/qcom/gcc-msm8960.c
index 051745ef99c8..56ce05a846dd 100644
--- a/drivers/clk/qcom/gcc-msm8960.c
+++ b/drivers/clk/qcom/gcc-msm8960.c
@@ -3624,7 +3624,7 @@  static int gcc_msm8960_probe(struct platform_device *pdev)
 	if (!match)
 		return -EINVAL;
 
-	ret = qcom_cc_register_board_clk(dev, "cxo_board", "cxo", 19200000);
+	ret = qcom_cc_register_board_clk(dev, "cxo-board", "cxo", 19200000);
 	if (ret)
 		return ret;