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

[Inochi Amaoto <inochiama@gmail.com>]

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.

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

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

> > +
> > +	return 0;
> > +}
> 
> Can you update Documentation/driver-api/phy/phy.rst with some terse
> references to the bulk API and its intended use? Not much, just say
> what it's for (like multi-lane protocols, and why some operations are
> missing: phy_validate(), phy_set_mode_ext() etc). I guess they are
> missing because currently they have no user, which is OK, but the rest
> of the world should be on the same page w.r.t. the future of this API.
> 
> Thanks!
> 

Yes, I will, this is something I missed. Thanks.

Regards,
Inochi

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy