linux-kernel - Re: [PATCH v2 2/3] i3c: master: Add Qualcomm I3C controller driver

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f6b0cec1-a20d-4ed8-84b2-06d464ae5e0c@quicinc.com>
Date: Fri, 28 Mar 2025 19:58:52 +0530
From: Mukesh Kumar Savaliya <quic_msavaliy@...cinc.com>
To: Krzysztof Kozlowski <krzk@...nel.org>, <alexandre.belloni@...tlin.com>,
        <robh@...nel.org>, <krzk+dt@...nel.org>, <conor+dt@...nel.org>,
        <jarkko.nikula@...ux.intel.com>, <linux-i3c@...ts.infradead.org>,
        <linux-arm-msm@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>, Trilok Soni <quic_tsoni@...cinc.com>
CC: <andersson@...nel.org>, <konradybcio@...nel.org>
Subject: Re: [PATCH v2 2/3] i3c: master: Add Qualcomm I3C controller driver

Thanks Krzysztof for correcting !

On 3/26/2025 7:58 PM, Krzysztof Kozlowski wrote:
> On 26/03/2025 15:16, Mukesh Kumar Savaliya wrote:
>> +
>> +static int i3c_geni_resources_init(struct geni_i3c_dev *gi3c, struct platform_device *pdev)
>> +{
>> +	int ret;
>> +
>> +	gi3c->se.base = devm_platform_ioremap_resource(pdev, 0);
>> +	if (IS_ERR(gi3c->se.base))
>> +		return PTR_ERR(gi3c->se.base);
>> +
>> +	gi3c->se.clk = devm_clk_get(&pdev->dev, "se-clk");
> 
> Never tested.
> 
sorry, i ran and i got below error now:
i3c@...000: Unevaluated properties are not allowed ('clock-names', 
'interrupts' were unexpected).

so i have made below change and ran dt_binding_check + dtbs_check, i 
could fix the issue. Let me have this review internally specific to 
dt-binding and will post next patch after internal review. So i can 
avoid such misses.

yaml :
+  clock-names:
+    const: se
+
-  - interrupts-extended
+  - interrupts

example:
clock-names = "se";

>> +	if (IS_ERR(gi3c->se.clk))
>> +		return dev_err_probe(&pdev->dev, PTR_ERR(gi3c->se.clk),
>> +							"Unable to get serial engine core clock: %pe\n",
>> +							gi3c->se.clk);
>> +
>> +	ret = device_property_read_u32(&pdev->dev, "se-clock-frequency", &gi3c->clk_src_freq);
> 
> I don't see previous comments implemented. Comment was: "Drop".
> 
> You did not test the DTS - again - even though I asked for that.
> 
Sorry for miss. I did test for this but what happened is while i worked 
for multiple other changes, i did miss merging of removal of 
se-clock-frequency from the driver code. I had hard-coded it instead of 
removing because we don't give this frequency selection to user right now.

> You claim you did internal review, but I have doubts because internal
> review would tell you how to test it (there is comprehensive internal
> guide - see go/upstream). Then the testing would point out this issue.
> 
> Such trivial things should not be in v1 even. But they happened so you
> got the review. Now you sent v2 ignoring that public review.
> 
> Sorry guys, please improve internal processes instead of wasting
> reviewers time.
> 
Sure, i agree there are misses. will have internal review with 
dt-bindings and related changes in driver, then will post V3.
> NAK.
> 
> Best regards,
> Krzysztof