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?
> Do you mean to say i should add this property in yaml too ?
Yes.
You cannot add undocumented ABI. That's a strong NAK.
> 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?
Best regards,
Krzysztof
