Re: [PATCH v4 4/5] phy: core: Add managed phy bulk data helper functions

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

On Tue, Sep 29, 2026 at 04:52:34PM +0800, Inochi Amaoto wrote:
> Add device managed variants of the phy bulk helper functions. So
> the driver can benefit from automatically managed phy handles.

...

> +int devm_of_phy_bulk_get_all(struct device *dev, struct device_node *np,
> +			     struct phy_bulk_data **phys)
> +{
> +	struct phy_bulk_devres *devres;
> +	int ret;

> +	*phys = NULL;

Why?! In case of error we modify the output, this is usually not the best
approach as in most of the cases the expectation is that whatever user
provide (including a garbage) should be left untouched in case of an error.

> +	if (!np)
> +		return 0;

Same here. On top why do we even care about np like this? Interestingly that
some other APIs consider this as np == dev_of_node(dev) case, and automatically
propagate that.

> +	devres = devres_alloc(devm_phy_bulk_release_all, sizeof(*devres),
> +			      GFP_KERNEL);
> +	if (!devres)
> +		return -ENOMEM;
> +
> +	ret = of_phy_bulk_get_all(np, &devres->phys);
> +	if (ret > 0) {
> +		for (int i = 0; i < ret; i++)
> +			phy_add_device_link(dev, devres->phys[i].phy);
> +		*phys = devres->phys;
> +		devres->num_phys = ret;
> +		devres_add(dev, devres);
> +	} else {
> +		devres_free(devres);
> +	}
> +
> +	return ret;
> +}

...

> +static inline int devm_of_phy_bulk_get_all(struct device *dev,
> +					   struct device_node *np,
> +					   struct phy_bulk_data **phys)
> +{
> +	if (phys)
> +		*phys = NULL;

Same as per above.

> +	return 0;

Why not an error? I do not see the _optional word in the function name.

> +}

-- 
With Best Regards,
Andy Shevchenko