Re: [PATCH v4 2/6] PCI: spacemit-k1: Add multiple PHY handles support

On Fri, Jul 10, 2026 at 06:55:10PM +0800, Inochi Amaoto wrote:

On Fri, Jul 10, 2026 at 11:07:40AM +0300, Andy Shevchenko wrote:

quoted

On Fri, Jul 10, 2026 at 09:57:05AM +0800, Inochi Amaoto wrote:

quoted

On Thu, Jul 09, 2026 at 10:16:28AM +0300, Andy Shevchenko wrote:

quoted

On Thu, Jul 09, 2026 at 12:00:22PM +0800, Inochi Amaoto wrote:

...

quoted

quoted

quoted

quoted

+	k1->phy_count = i;
+	if (k1->phy_count == 0)
+		return -EINVAL;
+
+	return 0;

This doesn't seem correct to me, I would expect phy_count to be assigned only
when it's valid. (Yes, perhaps 0 is the same as it was, but semantically it's
different 0 in this case.)

I guess you think 0 is a valid number? I can not understand what you thing
Assign this to 0 if there is no phy is fine to me, which shows there is 0
vaild phy found.

Isn't it already 0? Semantically code is wrong in a flow (not in the result).

In fact it is already 0 here. But I am not understand why you thing is wrong.
Could you explain it in detail? (Maybe you think it is not good to return 
-EINVAL?)

You rewrite 0 by 0, but the fact of the rewriting is inaccuracy.
We should not rewrite the default (whatever it is) with 0 count
as semantically they are different cases. The rule of thumb, we
don't assign values in case of errors, we leave them as is and
it's user / caller responsibility to assign the default and handle
errors properly. This is simple layering violation.

Your code should be

	if (i == 0)
		return -EINVAL;

	k1->phy_count = i;
	return 0;

quoted

quoted

quoted

See also above. Do we have some PHY API that just counts provided PHYs?
If not, that what you should probably add first, before this patch.

I have not found any api for this. But the actual problem is, how the api
is designed. I have checked both the array bulk api for reset and clock,
it seems like it is much more than this patch...

Yeah, I looked at the phy-core and I think it will be hard to implement.
So, the idea is then is to reallocate the pointer each time you get a new PHY.
In this case the phy_count will reflect the actual memory consumption by phy.

Emmm, I think it is kind of buggy and not necessary. In most case
this array is not long actually, so allocate some pointer should be
fine and be an acceptable cost.

Then the counted_by will be incorrect as it may access valid memory, but
unused by the driver.

...

quoted

quoted

quoted

quoted

+	for (i = 0; i < k1->phy_count; i++)

	for (unsigned int i = 0; i < k1->phy_count; i++)

I agree with the unsigned int, but I guess this definition is not
allowed in linux.

It's allowed and it's encouraged even by Linus. As long as iterator is local,
use this syntax sugar and reduce its scope. It hardens the code.

Could you give me a reference url to check,

Sure, there are two (one for integers and one for pointers)
https://lore.kernel.org/lkml/CAHk-=wiCOTW5UftUrAnvJkr6769D29tF7Of79gUjdQHS_TkF5A@mail.gmail.com/ (local)
https://lore.kernel.org/lkml/CAHk-=wgy8p4is8ApEQCT5NS7XFb+NXeo-TKz7jRRZVksLLBSrQ@mail.gmail.com/ (local)

I have not found this on the coding-style.
https://www.kernel.org/doc/html/latest/process/coding-style.html

Feel free to update the documentation.

quoted

quoted

quoted

quoted

+		phy_exit(k1->phy[i]);

-- 
With Best Regards,
Andy Shevchenko