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

[Inochi Amaoto]

On Mon, Sep 07, 2026 at 03:43:18PM +0300, Vladimir Oltean wrote:
> 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.
> 

I said it is more suitable as the phy subsystem already uses this, so
use "-ENOSYS" will have the same view of the existing code. But in fact
I think -EOPNOTSUPP is a better option as it provide the right 
information.

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

Yes, this is something confused me. I see nothing for this.

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

I think I can have a try by sending another patch. I think I should do
some search before doing a change. Thanks for this reminder.

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

Actually, I think this could be aligned with non stub code. I found some
of them return -ENODEV in some case. This could be make sense in some
case.

Regards
Inochi