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

[Andy Shevchenko]

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:

...

> > >  struct k1_pcie {
> > >  	struct dw_pcie pci;
> > >  	const struct k1_pcie_device_data *data;
> > > -	struct phy *phy;
> > > +	struct phy **phy;
> > 
> > Should it be annotated by __counted_by_ptr() ?
> 
> Yes, I think it can, this is something I have missed.
> 
> > > +	unsigned int phy_count;
> > 
> > Ah, you allocate much more memory than possible PHYs... Can you redesign and
> > use the above annotation?
> 
> IIRC use the annotation does not reduce this memory usage...

It's about how you allocate it, the code uses max_phy_count instead of
phy_count.

> > >  	void __iomem *link;
> > >  	struct regmap *pmu;	/* Errors ignored; MMIO-backed regmap */
> > >  	u32 pmu_off;
> > 
> > >  }

...

> > > +	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).

> > 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.

...

> > > +	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.

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

-- 
With Best Regards,
Andy Shevchenko