RE: [PATCH v5 1/2] phy: Add new PHY attribute max_link_rate and APIs to get/set PHY attributes

[Swapnil Kashinath Jakhade <sjakhade@cadence.com>]

Hi Laurent,

Thank you for your review comments.

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: Wednesday, September 2, 2020 5:56 AM
> To: Swapnil Kashinath Jakhade <sjakhade@cadence.com>
> Cc: vkoul@kernel.org; kishon@ti.com; linux-kernel@vger.kernel.org;
> maxime@cerno.tech; Milind Parab <mparab@cadence.com>; Yuti Suresh
> Amonkar <yamonkar@cadence.com>; nsekhar@ti.com;
> tomi.valkeinen@ti.com; jsarha@ti.com; praneeth@ti.com
> Subject: Re: [PATCH v5 1/2] phy: Add new PHY attribute max_link_rate and
> APIs to get/set PHY attributes
> 
> EXTERNAL MAIL
> 
> 
> Hi Swapnil,
> 
> Thank you for the patch.
> 
> On Mon, Aug 24, 2020 at 08:28:30PM +0200, Swapnil Jakhade wrote:
> > Add new PHY attribute max_link_rate to struct phy_attrs.
> > Add a pair of PHY APIs to get/set all the PHY attributes.
> > Use phy_get_attrs() to get attribute values and phy_set_attrs() to set
> > attribute values.
> >
> > Signed-off-by: Yuti Amonkar <yamonkar@cadence.com>
> > Signed-off-by: Swapnil Jakhade <sjakhade@cadence.com>
> > Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
> > ---
> >  include/linux/phy/phy.h | 43
> > +++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 43 insertions(+)
> >
> > diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h index
> > bcee8eba62b3..924cd1a3deea 100644
> > --- a/include/linux/phy/phy.h
> > +++ b/include/linux/phy/phy.h
> > @@ -115,10 +115,12 @@ struct phy_ops {
> >  /**
> >   * struct phy_attrs - represents phy attributes
> >   * @bus_width: Data path width implemented by PHY
> > + * @max_link_rate: Maximum link rate supported by PHY (in Mbps)
> >   * @mode: PHY mode
> >   */
> >  struct phy_attrs {
> >  	u32			bus_width;
> > +	u32			max_link_rate;
> >  	enum phy_mode		mode;
> >  };
> >
> > @@ -231,6 +233,37 @@ static inline void phy_set_bus_width(struct phy
> > *phy, int bus_width)  {
> >  	phy->attrs.bus_width = bus_width;
> >  }
> > +
> > +/**
> > + * phy_get_attrs() - get the values for PHY attributes.
> > + * @phy: the phy for which to get the attributes
> > + * @attrs: current PHY attributes that will be returned
> > + *
> > + * Intended to be used by phy consumers to get the current PHY
> > +attributes
> > + * stored in struct phy_attrs.
> > + */
> > +static inline void phy_get_attrs(struct phy *phy, struct phy_attrs
> > +*attrs) {
> > +	mutex_lock(&phy->mutex);
> > +	memcpy(attrs, &phy->attrs, sizeof(struct phy_attrs));
> > +	mutex_unlock(&phy->mutex);
> 
> Why is the mutex required, what does it guard against ?

Mutex protection is added in phy_get_attrs/phy_set_attrs APIs based on
review comments from Kishon for v3 of this patch series[1].
Also, please find below references to earlier versions of this patch series
and the discussions which are the basis for implementation in v5 of this
patch series.

v1 of the series can be found @ [2]
v2 of the series can be found @ [3]
v3 of the series can be found @ [4]
v4 of the series can be found @ [5]

[1] https://lkml.org/lkml/2020/7/13/529

[2] https://lkml.org/lkml/2020/4/28/140

[3] https://lkml.org/lkml/2020/5/26/507

[4] https://lkml.org/lkml/2020/7/13/380

[5] https://lkml.org/lkml/2020/7/17/158

> 
> > +}
> > +
> > +/**
> > + * phy_set_attrs() - set PHY attributes with new values.
> > + * @phy: the phy for which to set the attributes
> > + * @attrs: the &struct phy_attrs containing new PHY attributes to be
> > +set
> > + *
> > + * This can be used by PHY providers or PHY consumers to set the PHY
> > + * attributes. The locking is used to protect updating attributes
> > +when
> > + * PHY consumer is doing some PHY ops.
> > + */
> > +static inline void phy_set_attrs(struct phy *phy, const struct
> > +phy_attrs *attrs) {
> > +	mutex_lock(&phy->mutex);
> > +	memcpy(&phy->attrs, attrs, sizeof(struct phy_attrs));
> > +	mutex_unlock(&phy->mutex);
> > +}
> >  struct phy *phy_get(struct device *dev, const char *string);  struct
> > phy *phy_optional_get(struct device *dev, const char *string);  struct
> > phy *devm_phy_get(struct device *dev, const char *string); @@ -389,6
> > +422,16 @@ static inline void phy_set_bus_width(struct phy *phy, int
> bus_width)
> >  	return;
> >  }
> >
> > +static inline void phy_get_attrs(struct phy *phy, struct phy_attrs
> > +*attrs) {
> > +	return;
> 
> You can drop the return statement.

Okay.

> 
> > +}
> > +
> > +static inline void phy_set_attrs(struct phy *phy, const struct
> > +phy_attrs *attrs) {
> > +	return;
> 
> Here too.

Okay.

Thanks & regards,
Swapnil

> 
> > +}
> > +
> >  static inline struct phy *phy_get(struct device *dev, const char
> > *string)  {
> >  	return ERR_PTR(-ENOSYS);
> 
> --
> Regards,
> 
> Laurent Pinchart