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

[Mukesh Kumar Savaliya <quic_msavaliy@quicinc.com>]

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@884000: 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