Re: [PATCH 3/3] phy: core: Add phy bulk data helper functions

[Inochi Amaoto <inochiama@gmail.com>]

On Mon, Aug 31, 2026 at 12:28:59PM +0300, Andy Shevchenko wrote:
> On Mon, Aug 31, 2026 at 10:55:05AM +0800, Inochi Amaoto wrote:
> > Add several helper functions that allow drivers to get several phy
> > consumers in one operation. If any of the phy cannot be acquired then
> > any phys that were got will be put before returning to the caller.
> > 
> > This can relieve the driver owners' life who needs to handle many phys,
> > as well as each phy error reporting.
> 
> ...
> 
> > +/**
> > + * of_phy_get_parent_count() - Get the number of phys of a device node
> > + * @np: device_node for which to get the phy
> > + *
> > + * Return: the phy count if successful, 0 if no phy handle is found,
> > + * negative error value if error occurs.
> 
> Just for the reference, here is the correct format of the kernel-doc!
> 
> > + */
> > +static int of_phy_get_parent_count(const struct device_node *np)
> > +{
> > +	int count;
> > +
> > +	count = of_count_phandle_with_args(np, "phys", "#phy-cells");
> 
> > +	if (count == -ENOENT)
> > +		return 0;
> 
> Why? And if so, the function perhaps needs to return unsigned type. Also
> kernel-doc says about negative error codes.
> 

There is a special reason for -ENOENT is the of_count_phandle_with_args()
return -ENOENT if is can not find "phys" property. I think it is the case
that there is no phy required for this device. So I judge it and return
0. For other errors, they should not be translated so return them as they
are.

> > +	return count;
> > +}
> 
> ...
> 
> > +void phy_bulk_put(struct device *dev, int num_phys, struct phy_bulk_data *phys)
> > +{
> > +	if (!phys)
> > +		return;
> 
> > +	while (--num_phys >= 0) {
> 
> It's hard to follow.
> 
> 	while (num_phys--) {
> 
> will do the job. Ditto for other similar cases.
> 

Thanks.

> > +		if (phys[num_phys].phy)
> > +			phy_put(dev, phys[num_phys].phy);
> > +		phys[num_phys].phy = NULL;
> > +	}
> > +}
> 
> ...
> 
> > +static int __phy_bulk_get(struct device *dev, int num_phys,
> > +			  struct phy_bulk_data *phys, bool optional)
> > +{
> > +	int ret;
> 
> > +	int i;
> 
> Do you expect num_phys to be negative?
> 
> static int __phy_bulk_get(struct device *dev, unsigned int num_phys,
> ...
> 	unsigned int i;
> 
> Same comment to the rest of the similar changes.
> 

No, all should be postive, this is a mistake I have made, thanks
for pointing out.

> > +
> > +	for (i = 0; i < num_phys; i++)
> > +		phys[i].phy = NULL;
> > +
> > +	for (i = 0; i < num_phys; i++) {
> > +		phys[i].phy = phy_get(dev, phys[i].id);
> 
> 		ret = PTR_ERR_OR_ZERO(...);
> 
> > +		if (IS_ERR(phys[i].phy)) {
> 
> 		if (ret) {
> 
> > +			ret = PTR_ERR(phys[i].phy);
> > +			phys[i].phy = NULL;
> > +
> > +			if (ret == -ENODEV && optional)
> > +				continue;
> > +
> > +			dev_err_probe(dev, ret,
> > +				      "Failed to get phy: (%s)\n",
> 
> There is room on the previous line.
> 
> > +				      phys[i].id);
> > +			goto err;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +
> > +err:
> > +	phy_bulk_put(dev, i, phys);
> > +
> > +	return ret;
> > +}
> 
> ...
> 
> > + * Return: %0 if successful, a negative error code otherwise
> 
> Note, the reference to 0 is inconsistent with the previous changes.
> Make it there [of_phy_get_parent_count()] to follow.
> 

Thanks for this information.

> ...
> 
> > +static int of_phy_bulk_get_by_index(struct device_node *np, int num_phys,
> > +				    struct phy_bulk_data *phys)
> > +{
> > +	int ret, i;
> > +
> > +	for (i = 0; i < num_phys; i++) {
> > +		phys[i].id = NULL;
> > +		phys[i].phy = NULL;
> > +	}
> > +
> > +	for (i = 0; i < num_phys; i++) {
> > +		of_property_read_string_index(np, "phy-names", i,
> > +					      &phys[i].id);
> 
> The line limit is exactly 80, please fix your editor and double check that you
> use as much room as available (with the correction on the logical splits where
> it makes sense).
> 

Yes, you are right, I misjudge this as my completion plugin output the
arguments name. I will change that.

> > +
> > +		phys[i].phy = of_phy_get_by_index(np, i);
> 
> 		ret = PTR_ERR_OR_ZERO(...);
> 
> ?
> 

Right, I missed this.

> > +		if (IS_ERR(phys[i].phy)) {
> > +			ret = PTR_ERR(phys[i].phy);
> > +			phys[i].phy = NULL;
> > +			goto err;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +
> > +err:
> > +	of_phy_bulk_put(i, phys);
> > +
> > +	return ret;
> > +}
> 
> ...
> 
> > +int of_phy_bulk_get_all(struct device_node *np, struct phy_bulk_data **phys)
> > +{
> > +	struct phy_bulk_data *phy_bulk;
> > +	int num_phys;
> > +	int ret;
> > +
> > +	*phys = NULL;
> 
> > +	if (!np)
> > +		return 0;
> 
> Dup check? The OF APIs are usually NULL-aware.
> Same Q to the ress of the code.
> 

Thanks, I will remove them.

> > +	num_phys = of_phy_get_parent_count(np);
> > +	if (num_phys <= 0)
> > +		return num_phys;
> > +
> > +	phy_bulk = kmalloc_objs(*phy_bulk, num_phys);
> > +	if (!phy_bulk)
> > +		return -ENOMEM;
> > +
> > +	ret = of_phy_bulk_get_by_index(np, num_phys, phy_bulk);
> > +	if (ret) {
> > +		kfree(phy_bulk);
> > +		return ret;
> > +	}
> > +
> > +	*phys = phy_bulk;
> > +
> > +	return num_phys;
> > +}
> 
> ...
> 
> > +struct phy_bulk_devres {
> > +	struct phy_bulk_data *phys;
> > +	int num_phys;
> 
> Why signed?
> 

My mistake. It always needs to be unsigned.

> > +	bool free_phys;
> > +};
> 
> ...
> 
> I stopped here. It's too many stuff in a single patch. Please, split to two for
> a starter:
> - non-devm additions
> - devm coverage
> 

Sorry for a bad time, I will do a better check and fix them. And I will split
the patch into two in the next version.

Regards,
Inochi

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy