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

[Andy Shevchenko <andriy.shevchenko@linux.intel.com>]

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.

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

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

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

...

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

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

		ret = PTR_ERR_OR_ZERO(...);

?

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

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

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

-- 
With Best Regards,
Andy Shevchenko