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

[Inochi Amaoto]

On Sun, Jul 12, 2026 at 12:35:16PM +0300, Andy Shevchenko wrote:
> On Sun, Jul 12, 2026 at 01:41:25PM +0800, Inochi Amaoto wrote:
> > On Sat, Jul 11, 2026 at 03:44:10PM +0300, Andy Shevchenko wrote:
> > > 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:
> > > > > On Fri, Jul 10, 2026 at 09:57:05AM +0800, Inochi Amaoto wrote:
> > > > > > On Thu, Jul 09, 2026 at 10:16:28AM +0300, Andy Shevchenko wrote:
> > > > > > > On Thu, Jul 09, 2026 at 12:00:22PM +0800, Inochi Amaoto wrote:
> 
> ...
> 
> > > Then the counted_by will be incorrect as it may access valid memory, but
> > > unused by the driver.
> > 
> > After some search I found a requirement in GCC patch:
> > https://gcc.gnu.org/pipermail/gcc-patches/2024-May/653123.html
> > 
> > It seems like the array can have more elements than the counter.
> > Some I guess the reallocation is not necessary and the counter
> > is still correct. Correct me if I am wrong.
> 
> There will be no problem from memory accesses point of view, but there are two
> disadvantages:
> - the waste of memory for unused items
> - the memory accesses beyond used items, which may lead to or show the subtle
>   issues in the code elsewhere
> 
> Better to be correct. It might help debugging as well in some other cases.
> 
For correct number, I guess using "fwnode_property_get_reference_args"
is a idea, it is kind of ugly, but it does provide some function for
use to get the phy numbers. It is more like "of_clk_get_parent_count"
and "reset_control_get_count" in the clk/reset subsystems. 

I think adding a function like the above one is a good start point for
the phy array/bulk. At least this phy function is implementable for me,
since it is quite simple and straightforward.

Regards,
Inochi

> ...
> 
> > > > > > > > +	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@xxxxxxxxxxxxxx/
> > > https://lore.kernel.org/lkml/CAHk-=wgy8p4is8ApEQCT5NS7XFb+NXeo-TKz7jRRZVksLLBSrQ@xxxxxxxxxxxxxx/
> > > 
> > > > 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.
> > 
> > Good to know thanks.
> 
> Are you going to update documentation? I can review it.
> 
> > > > > > > > +		phy_exit(k1->phy[i]);
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>