Re: [PATCH 2/4] clk: qcom: lpassaudiocc-sc7280: Add support for LPASS resets for QCM6490

On 6/16/2024 1:19 PM, Krzysztof Kozlowski wrote:

On 10/06/2024 12:03, Taniya Das wrote:

quoted

On 5/31/2024 9:56 PM, Krzysztof Kozlowski wrote:

quoted

On 31/05/2024 12:22, Taniya Das wrote:

quoted

On the QCM6490 boards the LPASS firmware controls the complete clock
controller functionalities. But the LPASS resets are required to be
controlled from the high level OS. The Audio SW driver should be able to
assert/deassert the audio resets as required. Thus in clock driver add
support for the same.

Signed-off-by: Taniya Das <redacted>
---
   drivers/clk/qcom/lpassaudiocc-sc7280.c | 28 ++++++++++++++++++++++++++
   1 file changed, 28 insertions(+)

diff --git a/drivers/clk/qcom/lpassaudiocc-sc7280.c b/drivers/clk/qcom/lpassaudiocc-sc7280.c
index c43d0b1af7f7..7fdfd07c111c 100644
--- a/drivers/clk/qcom/lpassaudiocc-sc7280.c
+++ b/drivers/clk/qcom/lpassaudiocc-sc7280.c

@@ -1,6 +1,7 @@
   // SPDX-License-Identifier: GPL-2.0-only
   /*
    * Copyright (c) 2021, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights reserved.
    */
   
   #include <linux/clk-provider.h>

@@ -869,10 +870,36 @@ static struct platform_driver lpass_aon_cc_sc7280_driver = {
   	},
   };
   
+static const struct of_device_id lpass_audio_cc_qcm6490_match_table[] = {
+	{ .compatible = "qcom,qcm6490-lpassaudiocc" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, lpass_audio_cc_qcm6490_match_table);
+
+static int lpass_audio_cc_qcm6490_probe(struct platform_device *pdev)
+{
+	lpass_audio_cc_sc7280_regmap_config.name = "lpassaudio_cc_reset";
+	lpass_audio_cc_sc7280_regmap_config.max_register = 0xc8;
+
+	return qcom_cc_probe_by_index(pdev, 1, &lpass_audio_cc_reset_sc7280_desc);
+}
+
+static struct platform_driver lpass_audio_cc_qcm6490_driver = {
+	.probe = lpass_audio_cc_qcm6490_probe,
+	.driver = {
+		.name = "lpass_audio_cc-qcm6490",
+		.of_match_table = lpass_audio_cc_qcm6490_match_table,
+	},
+};
+
   static int __init lpass_audio_cc_sc7280_init(void)
   {
   	int ret;
   
+	ret = platform_driver_register(&lpass_audio_cc_qcm6490_driver);
+	if (ret)
+		return ret;
+
   	ret = platform_driver_register(&lpass_aon_cc_sc7280_driver);

Why this is a new platform driver?  There should be just one driver with
different match data.

The main problem for me is that the current board(QCM6490) needs to be
only support a subset of the entire(only resets) functionality the
SC7280. If I redesign the probe function to pick the match data then I
might accidentally break the existing functionalities on SC7280 boards.

That's not a reason to not implement changes. Test your changes first.

quoted

Hence I thought to have a separate driver registration which looked a
cleaner approach to go away from the "of_device_is_compatible".

No. You over complicate simple case introducing unusual pattern.

Krzysztof, now I am introducing a match data approach to differentiate 
between SC7280 and QCM6490 for adding the reset functionality only to 
the later board.

v2 series: 
https://lore.kernel.org/lkml/20240816-qcm6490-lpass-reset-v1-0-a11f33cad3c5@quicinc.com/T/#t (local) 

Best regards,
Krzysztof

-- 
Thanks & Regards,
Taniya Das.