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

[Inochi Amaoto <inochiama@gmail.com>]

On Wed, Sep 30, 2026 at 11:29:01AM +0300, Andy Shevchenko wrote:
> 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.
> 

This is something I think this is wrong and I need to removed.

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

After a deep recheck. I think you are true. We do not need to care
about that, just let the internal api decide whether it should be
an error is better. I misunderstand that it should check it at early
stage to avoid some bad use in the following logic. Now I found it
is meaningless.

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

IIRC get_all it a special optional meaning: treat not existing device as
0, this follows the same meaning like clk. Vladimir has a good explaination
on this in the previous version:
https://lore.kernel.org/linux-phy/20260907124318.6q4rr2huxyehm3zs@skbuf/

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