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

[Vladimir Oltean]

On Mon, Sep 07, 2026 at 08:15:04PM +0800, Inochi Amaoto wrote:
> On Mon, Sep 07, 2026 at 02:48:37PM +0300, Vladimir Oltean wrote:
> > On Fri, Sep 04, 2026 at 04:37:07PM +0800, Inochi Amaoto wrote:
> > > +static inline int phy_bulk_get_all(struct device *dev,
> > > +				   struct phy_bulk_data **phys)
> > > +{
> > > +	if (phys)
> > > +		*phys = NULL;
> > > +
> > > +	return -EOPNOTSUPP;
> > > +}
> > > +
> > > +static inline int of_phy_bulk_get_all(struct device_node *np,
> > > +				      struct phy_bulk_data **phys)
> > > +{
> > > +	if (phys)
> > > +		*phys = NULL;
> > > +
> > > +	return -EOPNOTSUPP;
> > > +}
> > 
> > Why do the stub definitions of *_get_all() return an error?
> > I would expect these to have optional semantics, i.e. 0 PHYs are not an
> > error to the consumer.
> > 
> > For reference, I am comparing with clk_bulk_get_all() which returns 0.
> 
> This is the thing I am not very clear to. I found the clk_bulk_get_all()
> return 0. But something in the reset return -EOPNOTSUPP for non optional
> get (I reference __reset_control_bulk_get, as reset does not have an
> API that is the same as this). I am not very sure whether it is best. 
> 
> Since you think we should follow this optional semantics, I think it is
> fine for me to change this to 0.

I am only talking about the *phy_bulk_get_all() functions, which have no
num_phys argument, and which as you said, have no reset_control equivalent.

The optional nature of *_get_all() should come specifically from the
fact that the consumer isn't specifically asking for "this many" PHYs
(num_phys). So the core can return 0 and say "that's all", and technically
not lie. Not a fatal error to the consumer; the bulk API supports other
calls with num_phys=0.

> > > +
> > > +static inline void phy_bulk_put(struct device *dev, unsigned int num_phys,
> > > +				struct phy_bulk_data *phys)
> > > +{
> > > +	if (!phys)
> > > +		return;
> > > +
> > > +	while (num_phys--)
> > > +		phys[num_phys].phy = NULL;
> > > +}
> > > +
> > > +static inline void of_phy_bulk_put(unsigned int num_phys,
> > > +				   struct phy_bulk_data *phys)
> > > +{
> > > +	if (!phys)
> > > +		return;
> > > +
> > > +	while (num_phys--)
> > > +		phys[num_phys].phy = NULL;
> > > +}
> > > +
> > > +static inline void phy_bulk_put_all(struct device *dev, unsigned int num_phys,
> > > +				    struct phy_bulk_data *phys)
> > > +{
> > > +	phy_bulk_put(dev, num_phys, phys);
> > > +}
> > > +
> > > +static inline void of_phy_bulk_put_all(unsigned int num_phys,
> > > +				       struct phy_bulk_data *phys)
> > > +{
> > > +	of_phy_bulk_put(num_phys, phys);
> > > +}
> > > +
> > > +static inline int phy_bulk_check_disabled(unsigned int num_phys,
> > > +					  struct phy_bulk_data *phys)
> > > +{
> > > +	if (!phys)
> > > +		return 0;
> > > +
> > > +	for (unsigned int i = 0; i < num_phys; i++)
> > > +		if (phys[i].phy)
> > > +			return -EOPNOTSUPP;
> > 
> > For consistency with the individual API, I believe this should be
> > -ENOSYS (not that I know why we would be using this error code).
> > 
> 
> In fact I think -ENOSYS is more suitable, but I found almost every
> subsystem use -EOPNOTSUPP for such a blob.

Why do you consider -ENOSYS to be more suitable? In include/uapi/asm-generic/errno.h
it says "/* Invalid system call number */" which makes it pretty use
case specific.

> So I think it will be
> good to follow a generic -EOPNOTSUPP. In fact I found nothing about
> why the phy subsystem use -ENOSYS for this, maybe someone can
> answer it.

It's been that way since initial commit ff764963479a ("drivers: phy: add
generic PHY framework") with no explanation.

> 
> Instead of switching to -ENOSYS, I think it could be more proper to
> change the existing blobs to -EOPNOTSUPP?

Personally I have nothing against this, though it depends on how deeply
you want to go in.

If you want to make this change, watch out for the following callers
which explicitly check for -ENOSYS:
- drivers/ata/libahci_platform.c:374
- drivers/usb/dwc2/platform.c:246
- drivers/usb/dwc3/core.c:1591,1608
- drivers/gpu/drm/bridge/analogix/analogix_dp_core.c:1363

Only devm_phy_get() / devm_of_phy_get() return codes get parsed this way.
For the runtime consumer functions, all consumers seem -ENOSYS-unaware.

Though after seeing how some drivers treat -ENODEV (for an absent PHY)
and -ENOSYS (for the disabled Generic PHY framework) the same, I think
it might make more sense to return -ENODEV from the stubs.