Re: [PATCH v1 2/3] i3c: master: Add Qualcomm I3C master controller driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Thanks Krzysztof !

On 2/9/2025 5:10 PM, Krzysztof Kozlowski wrote:
On 07/02/2025 13:03, Mukesh Kumar Savaliya wrote:
+	gi3c->se.clk = devm_clk_get(&pdev->dev, "se-clk");
+	if (IS_ERR(gi3c->se.clk)) {
+		ret = PTR_ERR(gi3c->se.clk);
+		dev_err(&pdev->dev, "Error getting SE Core clk %d\n", ret);
+		return ret;
+	}
+
+	ret = device_property_read_u32(&pdev->dev, "se-clock-frequency", &gi3c->clk_src_freq);

You never tested your DTS or this code... Drop

I have tested on SM8550 MTP only. Below entry in my internal/local DTSI.


And how is it supposed to work? Are you going to send us your local
internal DTSI? Is it going to pass any checks?
was saying about code was testing with MTP. DTS was tested using dt-bindings check.
I should add "se-clock-frequency" and "dfs-index"

Do you mean to say i should add this property in yaml too ?

Yes.

You cannot add undocumented ABI. That's a strong NAK.

sure , adding both properties into yaml.

se-clock-frequency = <100000000>;

+	if (ret) {
+		dev_info(&pdev->dev, "SE clk freq not specified, default to 100 MHz.\n");
+		gi3c->clk_src_freq = 100000000;
+	}
+
+	ret = geni_icc_get(&gi3c->se, NULL);
+	if (ret)
+		return ret;
+
+	/* Set the bus quota to a reasonable value for register access */
+	gi3c->se.icc_paths[GENI_TO_CORE].avg_bw = GENI_DEFAULT_BW;
+	gi3c->se.icc_paths[CPU_TO_GENI].avg_bw = GENI_DEFAULT_BW;
+	ret = geni_icc_set_bw(&gi3c->se);
+	if (ret) {
+		dev_err(&pdev->dev, "%s: icc set bw failed ret:%d\n", __func__, ret);
+		return ret;
+	}
+	dev_dbg(&pdev->dev, "%s: GENI_TO_CORE:%d CPU_TO_GENI:%d\n", __func__,
+		gi3c->se.icc_paths[GENI_TO_CORE].avg_bw, gi3c->se.icc_paths[CPU_TO_GENI].avg_bw);
+
+	ret = device_property_read_u32(&pdev->dev, "dfs-index", &gi3c->dfs_idx);

The same. You cannot send us hidden ABI.

This code does not look like ready for upstream. Are you sure it was
internally reviewed?

yes, we have taken 2 rounds internally.


And none of the reviewers spotted undocumented ABI? OK, learning
experience for me.


Are you saying i should add this into yaml ?  what do you mean by
hiddern ABI ?


Where is the documentation of the ABI?

can you please point to other ABI document so i can prepare and share it.


Best regards,
Krzysztof





[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux