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 05:21:08PM +0800, Inochi Amaoto wrote:
> 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/
> 

s/device/property/.

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