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

[Inochi Amaoto]

On Wed, Sep 30, 2026 at 11:22:32AM +0300, Andy Shevchenko wrote:
> On Tue, Sep 29, 2026 at 04:52:33PM +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_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.
> > + */
> > +static int of_phy_get_count(const struct device_node *np)
> > +{
> > +	int count;
> > +
> > +	count = of_count_phandle_with_args(np, "phys", "#phy-cells");
> 
> > +
> 
> I would drop this blank line.
> 
> > +	if (count == -ENOENT)
> > +		return 0;
> 
> I'm not sure about usefulness of this kind of trick in the _count API.
> 
> > +	return count;
> > +}
> 
> ...
> 
> > +static void phy_bulk_put(struct device *dev, unsigned int num_phys,
> > +			 struct phy_bulk_data *phys)
> > +{
> > +	while (num_phys--) {
> 
> > +		if (!IS_ERR_OR_NULL(phys[num_phys].phy))
> 
> This should be part of phy_put(). In general many kernel resource release APIs
> are NULL and/or error pointer aware. This is a slow path and it makes user's life
> easier
> 

Something reasonable, As of_phy_put does have a check, but phy_put
does not and it use phy->dev directly. I think this is acceptable
QoL change.

> > +			phy_put(dev, phys[num_phys].phy);
> > +		phys[num_phys].phy = NULL;
> > +	}
> > +}
> 
> ...
> 
> > +static void of_phy_bulk_put(unsigned int num_phys, struct phy_bulk_data *phys)
> > +{
> > +	while (num_phys--) {
> > +		of_phy_put(phys[num_phys].phy);
> 
> > +		phys[num_phys].phy = NULL;
> 
> Is NULLification mandatory?
> 

I think it may not be as the phy is managed internally.
But I think it could be keeped to avoid misuse.

> > +	}
> > +}
> 
> ...
> 
> > +static int of_phy_bulk_get_by_index(struct device_node *np,
> > +				    unsigned int num_phys,
> > +				    struct phy_bulk_data *phys)
> > +{
> > +	unsigned int i;
> > +	int ret;
> > +
> > +	for (i = 0; i < num_phys; i++) {
> > +		phys[i].id = NULL;
> > +		phys[i].phy = NULL;
> > +	}
> 
> But why? The below does the assognments.
> 

I refered of_clk_bulk_get(). And at least I think the init for
phy field should be kept for an initial state. For id, I think
it is possible to be removed.

> > +	for (i = 0; i < num_phys; i++) {
> > +		of_property_read_string_index(np, "phy-names", i, &phys[i].id);
> 
> > +		phys[i].phy = of_phy_get_by_index(np, i);
> > +
> > +		ret = PTR_ERR_OR_ZERO(phys[i].phy);
> > +		if (ret) {
> > +			phys[i].phy = NULL;
> 
> Same Q: do we need a NULLification in this case? Perhaps the respective APIs
> should be error pointer aware?
> 

I think this should be removed to keep the error info. This is the
thing I have missed. Thanks.

> > +			goto err;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +
> > +err:
> > +	of_phy_bulk_put(i, phys);
> > +
> > +	return ret;
> > +}
> 
> ...
> 
> > +/**
> > + * phy_bulk_exit() - exit multiple PHYs
> > + * @num_phys: number of entries in the phys array
> > + * @phys: array of struct phy_bulk_data to exit
> > + *
> > + * Exits the PHYs in reverse array order. All PHYs are processed even if an
> > + * error occurs.
> > + *
> > + * Return: %0 if successful, the first negative error code otherwise
> > + */
> > +int phy_bulk_exit(unsigned int num_phys, struct phy_bulk_data *phys)
> > +{
> > +	int ret = 0;
> > +	int err;
> > +
> > +	while (num_phys--) {
> > +		err = phy_exit(phys[num_phys].phy);
> > +		if (err && !ret)
> > +			ret = err;
> > +	}
> > +
> > +	return ret;
> 
> So, we return an arbitrary error and inconsistent state of the phys[] array.
> What can caller do about all this? Any type of recovery? TL;DR:
> I put in doubt the function prototype and the implementation (error handling).
> 

This is something I have done wrongly. It should return when the first
error occurs. So the caller can continue to exit the left phys.

As the phy_exit() does not check whether the init_count is 0, I think 
an additional check is needed fpr phy_exit() to avoid a negative 
init_count.

> > +}
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>