[PATCH v5 0/2] Add new PHY APIs to framework to get/set PHY attributes

[PATCH v5 0/2] Add new PHY APIs to framework to get/set PHY attributes

[Swapnil Jakhade]

This patch series adds a new pair of PHY APIs that can be used to get/set
all the PHY attributes. It also adds a new PHY attribute max_link_rate.

It includes following patches:

1. v5-0001-phy-Add-new-PHY-attribute-max_link_rate-and-APIs-.patch
This patch adds max_link_rate as a new PHY attribute along with a pair of
APIs that allow using the generic PHY subsystem to get/set PHY attributes
supported by the PHY.

2. v5-0002-phy-cadence-torrent-Use-kernel-PHY-API-to-set-PHY.patch
This patch uses PHY API phy_set_attrs() to set corresponding PHY properties
in Cadence Torrent PHY driver. This will enable drivers using this PHY to
read these properties using PHY framework.

The phy_get_attrs() API will be used in the DRM bridge driver [1] which is
in the process of upstreaming.

[1]

https://lkml.org/lkml/2020/8/6/649

Version History:

v5:
    - Add kernel-doc comments for phy_get_attrs/phy_set_attrs APIs
    - Pass second parameter of phy_set_attrs() as const struct *
    - Add Acked-by: Kishon Vijay Abraham I <kishon@ti.com>

v4:
    - Protect phy_get_attrs/phy_set_attrs APIs with mutex

v3:
    - Add comment describing new PHY attribute max_link_rate
    - Use of memcpy to copy structure members
    - Change commit log a bit

v2:
    - Implemented single pair of functions to get/set all PHY attributes

Swapnil Jakhade (2):
  phy: Add new PHY attribute max_link_rate and APIs to get/set PHY
    attributes
  phy: cadence-torrent: Use kernel PHY API to set PHY attributes

 drivers/phy/cadence/phy-cadence-torrent.c |  7 ++++
 include/linux/phy/phy.h                   | 43 +++++++++++++++++++++++
 2 files changed, 50 insertions(+)

-- 
2.26.1

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

[Swapnil Jakhade]

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);
+}
+
+/**
+ * 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;
+}
+
+static inline void phy_set_attrs(struct phy *phy, const struct phy_attrs *attrs)
+{
+	return;
+}
+
 static inline struct phy *phy_get(struct device *dev, const char *string)
 {
 	return ERR_PTR(-ENOSYS);
-- 
2.26.1

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

[Laurent Pinchart]

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 ?

> +}
> +
> +/**
> + * 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.

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

Here too.

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

-- 
Regards,

Laurent Pinchart

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

[Swapnil Kashinath Jakhade]

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

[PATCH v5 2/2] phy: cadence-torrent: Use kernel PHY API to set PHY attributes

[Swapnil Jakhade]

Use generic PHY framework function phy_set_attrs() to set number
of lanes and maximum link rate supported by PHY.

Signed-off-by: Swapnil Jakhade <sjakhade@cadence.com>
Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
---
 drivers/phy/cadence/phy-cadence-torrent.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/phy/cadence/phy-cadence-torrent.c b/drivers/phy/cadence/phy-cadence-torrent.c
index 7116127358ee..eca71467c4a8 100644
--- a/drivers/phy/cadence/phy-cadence-torrent.c
+++ b/drivers/phy/cadence/phy-cadence-torrent.c
@@ -1710,6 +1710,7 @@ static int cdns_torrent_phy_probe(struct platform_device *pdev)
 	struct cdns_torrent_phy *cdns_phy;
 	struct device *dev = &pdev->dev;
 	struct phy_provider *phy_provider;
+	struct phy_attrs torrent_attr;
 	const struct of_device_id *match;
 	struct cdns_torrent_data *data;
 	struct device_node *child;
@@ -1852,6 +1853,12 @@ static int cdns_torrent_phy_probe(struct platform_device *pdev)
 				 cdns_phy->phys[node].num_lanes,
 				 cdns_phy->max_bit_rate / 1000,
 				 cdns_phy->max_bit_rate % 1000);
+
+			torrent_attr.bus_width = cdns_phy->phys[node].num_lanes;
+			torrent_attr.max_link_rate = cdns_phy->max_bit_rate;
+			torrent_attr.mode = PHY_MODE_DP;
+
+			phy_set_attrs(gphy, &torrent_attr);
 		} else {
 			dev_err(dev, "Driver supports only PHY_TYPE_DP\n");
 			ret = -ENOTSUPP;
-- 
2.26.1

Re: [PATCH v5 2/2] phy: cadence-torrent: Use kernel PHY API to set PHY attributes

[Laurent Pinchart]

Hi Swapnil,

Thank you for the patch.

On Mon, Aug 24, 2020 at 08:28:31PM +0200, Swapnil Jakhade wrote:
> Use generic PHY framework function phy_set_attrs() to set number
> of lanes and maximum link rate supported by PHY.
> 
> Signed-off-by: Swapnil Jakhade <sjakhade@cadence.com>
> Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
> ---
>  drivers/phy/cadence/phy-cadence-torrent.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/phy/cadence/phy-cadence-torrent.c b/drivers/phy/cadence/phy-cadence-torrent.c
> index 7116127358ee..eca71467c4a8 100644
> --- a/drivers/phy/cadence/phy-cadence-torrent.c
> +++ b/drivers/phy/cadence/phy-cadence-torrent.c
> @@ -1710,6 +1710,7 @@ static int cdns_torrent_phy_probe(struct platform_device *pdev)
>  	struct cdns_torrent_phy *cdns_phy;
>  	struct device *dev = &pdev->dev;
>  	struct phy_provider *phy_provider;
> +	struct phy_attrs torrent_attr;
>  	const struct of_device_id *match;
>  	struct cdns_torrent_data *data;
>  	struct device_node *child;
> @@ -1852,6 +1853,12 @@ static int cdns_torrent_phy_probe(struct platform_device *pdev)
>  				 cdns_phy->phys[node].num_lanes,
>  				 cdns_phy->max_bit_rate / 1000,
>  				 cdns_phy->max_bit_rate % 1000);
> +
> +			torrent_attr.bus_width = cdns_phy->phys[node].num_lanes;
> +			torrent_attr.max_link_rate = cdns_phy->max_bit_rate;
> +			torrent_attr.mode = PHY_MODE_DP;
> +
> +			phy_set_attrs(gphy, &torrent_attr);

Why is this better than accessing the attributes manually as follows ?

			gphy->attrs.bus_width = cdns_phy->phys[node].num_lanes;
			gphy->attrs.max_link_rate = cdns_phy->max_bit_rate;
			gphy->attrs.mode = PHY_MODE_DP;

This is called in cdns_torrent_phy_probe(), before the PHY provider is
registered, so nothing can access the PHY yet. What race condition are
you trying to protect against with usage of phy_set_attrs() ?

>  		} else {
>  			dev_err(dev, "Driver supports only PHY_TYPE_DP\n");
>  			ret = -ENOTSUPP;

-- 
Regards,

Laurent Pinchart

RE: [PATCH v5 2/2] phy: cadence-torrent: Use kernel PHY API to set PHY attributes

[Swapnil Kashinath Jakhade]

Hi Laurent,

Thank you for your review comments.

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: Wednesday, September 2, 2020 6:00 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 2/2] phy: cadence-torrent: Use kernel PHY API to set
> PHY attributes
> 
> EXTERNAL MAIL
> 
> 
> Hi Swapnil,
> 
> Thank you for the patch.
> 
> On Mon, Aug 24, 2020 at 08:28:31PM +0200, Swapnil Jakhade wrote:
> > Use generic PHY framework function phy_set_attrs() to set number of
> > lanes and maximum link rate supported by PHY.
> >
> > Signed-off-by: Swapnil Jakhade <sjakhade@cadence.com>
> > Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
> > ---
> >  drivers/phy/cadence/phy-cadence-torrent.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> >
> > diff --git a/drivers/phy/cadence/phy-cadence-torrent.c
> > b/drivers/phy/cadence/phy-cadence-torrent.c
> > index 7116127358ee..eca71467c4a8 100644
> > --- a/drivers/phy/cadence/phy-cadence-torrent.c
> > +++ b/drivers/phy/cadence/phy-cadence-torrent.c
> > @@ -1710,6 +1710,7 @@ static int cdns_torrent_phy_probe(struct
> platform_device *pdev)
> >  	struct cdns_torrent_phy *cdns_phy;
> >  	struct device *dev = &pdev->dev;
> >  	struct phy_provider *phy_provider;
> > +	struct phy_attrs torrent_attr;
> >  	const struct of_device_id *match;
> >  	struct cdns_torrent_data *data;
> >  	struct device_node *child;
> > @@ -1852,6 +1853,12 @@ static int cdns_torrent_phy_probe(struct
> platform_device *pdev)
> >  				 cdns_phy->phys[node].num_lanes,
> >  				 cdns_phy->max_bit_rate / 1000,
> >  				 cdns_phy->max_bit_rate % 1000);
> > +
> > +			torrent_attr.bus_width = cdns_phy-
> >phys[node].num_lanes;
> > +			torrent_attr.max_link_rate = cdns_phy-
> >max_bit_rate;
> > +			torrent_attr.mode = PHY_MODE_DP;
> > +
> > +			phy_set_attrs(gphy, &torrent_attr);
> 
> Why is this better than accessing the attributes manually as follows ?
> 
> 			gphy->attrs.bus_width = cdns_phy-
> >phys[node].num_lanes;
> 			gphy->attrs.max_link_rate = cdns_phy-
> >max_bit_rate;
> 			gphy->attrs.mode = PHY_MODE_DP;
> 
> This is called in cdns_torrent_phy_probe(), before the PHY provider is
> registered, so nothing can access the PHY yet. What race condition are you
> trying to protect against with usage of phy_set_attrs() ?

I agree that for Cadence DP bridge driver and Torrent PHY driver use case, it
would not matter even if we set the attributes in Torrent PHY driver in a way
you suggested above.
But as per the discussion in [1], phy_set_attrs/phy_get_attrs APIs in future could
maybe used by other drivers replacing existing individual functions for attributes
bus_width and mode which are phy_set_bus_width/phy_get_bus_width and
phy_set_mode/phy_get_mode. So this usage in Torrent PHY driver is an example
implementation of the API.

[1] https://lkml.org/lkml/2020/5/18/472

Thanks & regards,
Swapnil

> 
> >  		} else {
> >  			dev_err(dev, "Driver supports only
> PHY_TYPE_DP\n");
> >  			ret = -ENOTSUPP;
> 
> --
> Regards,
> 
> Laurent Pinchart

Re: [PATCH v5 2/2] phy: cadence-torrent: Use kernel PHY API to set PHY attributes

[Laurent Pinchart]

Hi Swapnil,

On Wed, Sep 02, 2020 at 07:09:21AM +0000, Swapnil Kashinath Jakhade wrote:
> On Wednesday, September 2, 2020 6:00 AM Laurent Pinchart wrote:
> > On Mon, Aug 24, 2020 at 08:28:31PM +0200, Swapnil Jakhade wrote:
> > > Use generic PHY framework function phy_set_attrs() to set number of
> > > lanes and maximum link rate supported by PHY.
> > >
> > > Signed-off-by: Swapnil Jakhade <sjakhade@cadence.com>
> > > Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
> > > ---
> > >  drivers/phy/cadence/phy-cadence-torrent.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > >
> > > diff --git a/drivers/phy/cadence/phy-cadence-torrent.c
> > > b/drivers/phy/cadence/phy-cadence-torrent.c
> > > index 7116127358ee..eca71467c4a8 100644
> > > --- a/drivers/phy/cadence/phy-cadence-torrent.c
> > > +++ b/drivers/phy/cadence/phy-cadence-torrent.c
> > > @@ -1710,6 +1710,7 @@ static int cdns_torrent_phy_probe(struct platform_device *pdev)
> > >  	struct cdns_torrent_phy *cdns_phy;
> > >  	struct device *dev = &pdev->dev;
> > >  	struct phy_provider *phy_provider;
> > > +	struct phy_attrs torrent_attr;
> > >  	const struct of_device_id *match;
> > >  	struct cdns_torrent_data *data;
> > >  	struct device_node *child;
> > > @@ -1852,6 +1853,12 @@ static int cdns_torrent_phy_probe(struct platform_device *pdev)
> > >  				 cdns_phy->phys[node].num_lanes,
> > >  				 cdns_phy->max_bit_rate / 1000,
> > >  				 cdns_phy->max_bit_rate % 1000);
> > > +
> > > +			torrent_attr.bus_width = cdns_phy- >phys[node].num_lanes;
> > > +			torrent_attr.max_link_rate = cdns_phy->max_bit_rate;
> > > +			torrent_attr.mode = PHY_MODE_DP;
> > > +
> > > +			phy_set_attrs(gphy, &torrent_attr);
> > 
> > Why is this better than accessing the attributes manually as follows ?
> > 
> > 			gphy->attrs.bus_width = cdns_phy->phys[node].num_lanes;
> > 			gphy->attrs.max_link_rate = cdns_phy->max_bit_rate;
> > 			gphy->attrs.mode = PHY_MODE_DP;
> > 
> > This is called in cdns_torrent_phy_probe(), before the PHY provider is
> > registered, so nothing can access the PHY yet. What race condition are you
> > trying to protect against with usage of phy_set_attrs() ?
> 
> I agree that for Cadence DP bridge driver and Torrent PHY driver use case, it
> would not matter even if we set the attributes in Torrent PHY driver in a way
> you suggested above.
> But as per the discussion in [1], phy_set_attrs/phy_get_attrs APIs in future could
> maybe used by other drivers replacing existing individual functions for attributes
> bus_width and mode which are phy_set_bus_width/phy_get_bus_width and
> phy_set_mode/phy_get_mode. So this usage in Torrent PHY driver is an example
> implementation of the API.
> 
> [1] https://lkml.org/lkml/2020/5/18/472

This doesn't seem a very good API to me :-S It will require callers to
always call phy_get_attrs() first, modify the attributes they want to
set, and then call phy_set_attrs(). Not only will be copy the whole
phy_attrs structure needlessly, it will also not be an atomic operation
as someone else could modify attributes between the get and set calls.
The lack of atomicity may not be an issue in practice if there's a
single user of the PHY at all times, but in that case no mutex is
needed.

I think this series tries to fix a problem that doesn't exist.

> > >  		} else {
> > >  			dev_err(dev, "Driver supports only PHY_TYPE_DP\n");
> > >  			ret = -ENOTSUPP;

-- 
Regards,

Laurent Pinchart

RE: [PATCH v5 2/2] phy: cadence-torrent: Use kernel PHY API to set PHY attributes

[Swapnil Kashinath Jakhade]

> -----Original Message-----
> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Sent: Wednesday, September 2, 2020 5:47 PM
> 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 2/2] phy: cadence-torrent: Use kernel PHY API to set
> PHY attributes
> 
> EXTERNAL MAIL
> 
> 
> Hi Swapnil,
> 
> On Wed, Sep 02, 2020 at 07:09:21AM +0000, Swapnil Kashinath Jakhade
> wrote:
> > On Wednesday, September 2, 2020 6:00 AM Laurent Pinchart wrote:
> > > On Mon, Aug 24, 2020 at 08:28:31PM +0200, Swapnil Jakhade wrote:
> > > > Use generic PHY framework function phy_set_attrs() to set number
> > > > of lanes and maximum link rate supported by PHY.
> > > >
> > > > Signed-off-by: Swapnil Jakhade <sjakhade@cadence.com>
> > > > Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
> > > > ---
> > > >  drivers/phy/cadence/phy-cadence-torrent.c | 7 +++++++
> > > >  1 file changed, 7 insertions(+)
> > > >
> > > > diff --git a/drivers/phy/cadence/phy-cadence-torrent.c
> > > > b/drivers/phy/cadence/phy-cadence-torrent.c
> > > > index 7116127358ee..eca71467c4a8 100644
> > > > --- a/drivers/phy/cadence/phy-cadence-torrent.c
> > > > +++ b/drivers/phy/cadence/phy-cadence-torrent.c
> > > > @@ -1710,6 +1710,7 @@ static int cdns_torrent_phy_probe(struct
> platform_device *pdev)
> > > >  	struct cdns_torrent_phy *cdns_phy;
> > > >  	struct device *dev = &pdev->dev;
> > > >  	struct phy_provider *phy_provider;
> > > > +	struct phy_attrs torrent_attr;
> > > >  	const struct of_device_id *match;
> > > >  	struct cdns_torrent_data *data;
> > > >  	struct device_node *child;
> > > > @@ -1852,6 +1853,12 @@ static int cdns_torrent_phy_probe(struct
> platform_device *pdev)
> > > >  				 cdns_phy->phys[node].num_lanes,
> > > >  				 cdns_phy->max_bit_rate / 1000,
> > > >  				 cdns_phy->max_bit_rate % 1000);
> > > > +
> > > > +			torrent_attr.bus_width = cdns_phy-
> >phys[node].num_lanes;
> > > > +			torrent_attr.max_link_rate = cdns_phy-
> >max_bit_rate;
> > > > +			torrent_attr.mode = PHY_MODE_DP;
> > > > +
> > > > +			phy_set_attrs(gphy, &torrent_attr);
> > >
> > > Why is this better than accessing the attributes manually as follows ?
> > >
> > > 			gphy->attrs.bus_width = cdns_phy-
> >phys[node].num_lanes;
> > > 			gphy->attrs.max_link_rate = cdns_phy-
> >max_bit_rate;
> > > 			gphy->attrs.mode = PHY_MODE_DP;
> > >
> > > This is called in cdns_torrent_phy_probe(), before the PHY provider
> > > is registered, so nothing can access the PHY yet. What race
> > > condition are you trying to protect against with usage of phy_set_attrs() ?
> >
> > I agree that for Cadence DP bridge driver and Torrent PHY driver use
> > case, it would not matter even if we set the attributes in Torrent PHY
> > driver in a way you suggested above.
> > But as per the discussion in [1], phy_set_attrs/phy_get_attrs APIs in
> > future could maybe used by other drivers replacing existing individual
> > functions for attributes bus_width and mode which are
> > phy_set_bus_width/phy_get_bus_width and
> phy_set_mode/phy_get_mode. So
> > this usage in Torrent PHY driver is an example implementation of the API.
> >
> > [1]
> > https://urldefense.com/v3/__https://lkml.org/lkml/2020/5/18/472__;!!EH
> > scmS1ygiU1lA!QKTTI7BS1R35a_zoMfJsY4A4yCtEKrQNtiAXTyIZ-
> SYIEEibYdpBMJTll
> > Yrd-00$
> 
> This doesn't seem a very good API to me :-S It will require callers to always
> call phy_get_attrs() first, modify the attributes they want to set, and then call
> phy_set_attrs(). Not only will be copy the whole phy_attrs structure
> needlessly, it will also not be an atomic operation as someone else could
> modify attributes between the get and set calls.
> The lack of atomicity may not be an issue in practice if there's a single user of
> the PHY at all times, but in that case no mutex is needed.
> 
> I think this series tries to fix a problem that doesn't exist.

Thanks Laurent for your comments.

Hi Kishon,

Could you please suggest what would be the better approach regarding this PHY
attributes series. Should we add individual get/set functions for new attribute
max_link_rate just like mode and bus_width, or should we use phy_get_attrs()
and phy_set_attrs() functions removing mutex.  Your suggestions would really help.

Thanks & regards,
Swapnil

> 
> > > >  		} else {
> > > >  			dev_err(dev, "Driver supports only
> PHY_TYPE_DP\n");
> > > >  			ret = -ENOTSUPP;
> 
> --
> Regards,
> 
> Laurent Pinchart

Re: [PATCH v5 2/2] phy: cadence-torrent: Use kernel PHY API to set PHY attributes

[Kishon Vijay Abraham I]

Hi Swapnil,

On 9/3/2020 4:29 PM, Swapnil Kashinath Jakhade wrote:
> 
> 
>> -----Original Message-----
>> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Sent: Wednesday, September 2, 2020 5:47 PM
>> 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 2/2] phy: cadence-torrent: Use kernel PHY API to set
>> PHY attributes
>>
>> EXTERNAL MAIL
>>
>>
>> Hi Swapnil,
>>
>> On Wed, Sep 02, 2020 at 07:09:21AM +0000, Swapnil Kashinath Jakhade
>> wrote:
>>> On Wednesday, September 2, 2020 6:00 AM Laurent Pinchart wrote:
>>>> On Mon, Aug 24, 2020 at 08:28:31PM +0200, Swapnil Jakhade wrote:
>>>>> Use generic PHY framework function phy_set_attrs() to set number
>>>>> of lanes and maximum link rate supported by PHY.
>>>>>
>>>>> Signed-off-by: Swapnil Jakhade <sjakhade@cadence.com>
>>>>> Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>>> ---
>>>>>  drivers/phy/cadence/phy-cadence-torrent.c | 7 +++++++
>>>>>  1 file changed, 7 insertions(+)
>>>>>
>>>>> diff --git a/drivers/phy/cadence/phy-cadence-torrent.c
>>>>> b/drivers/phy/cadence/phy-cadence-torrent.c
>>>>> index 7116127358ee..eca71467c4a8 100644
>>>>> --- a/drivers/phy/cadence/phy-cadence-torrent.c
>>>>> +++ b/drivers/phy/cadence/phy-cadence-torrent.c
>>>>> @@ -1710,6 +1710,7 @@ static int cdns_torrent_phy_probe(struct
>> platform_device *pdev)
>>>>>  	struct cdns_torrent_phy *cdns_phy;
>>>>>  	struct device *dev = &pdev->dev;
>>>>>  	struct phy_provider *phy_provider;
>>>>> +	struct phy_attrs torrent_attr;
>>>>>  	const struct of_device_id *match;
>>>>>  	struct cdns_torrent_data *data;
>>>>>  	struct device_node *child;
>>>>> @@ -1852,6 +1853,12 @@ static int cdns_torrent_phy_probe(struct
>> platform_device *pdev)
>>>>>  				 cdns_phy->phys[node].num_lanes,
>>>>>  				 cdns_phy->max_bit_rate / 1000,
>>>>>  				 cdns_phy->max_bit_rate % 1000);
>>>>> +
>>>>> +			torrent_attr.bus_width = cdns_phy-
>>> phys[node].num_lanes;
>>>>> +			torrent_attr.max_link_rate = cdns_phy-
>>> max_bit_rate;
>>>>> +			torrent_attr.mode = PHY_MODE_DP;
>>>>> +
>>>>> +			phy_set_attrs(gphy, &torrent_attr);
>>>>
>>>> Why is this better than accessing the attributes manually as follows ?
>>>>
>>>> 			gphy->attrs.bus_width = cdns_phy-
>>> phys[node].num_lanes;
>>>> 			gphy->attrs.max_link_rate = cdns_phy-
>>> max_bit_rate;
>>>> 			gphy->attrs.mode = PHY_MODE_DP;
>>>>
>>>> This is called in cdns_torrent_phy_probe(), before the PHY provider
>>>> is registered, so nothing can access the PHY yet. What race
>>>> condition are you trying to protect against with usage of phy_set_attrs() ?
>>>
>>> I agree that for Cadence DP bridge driver and Torrent PHY driver use
>>> case, it would not matter even if we set the attributes in Torrent PHY
>>> driver in a way you suggested above.
>>> But as per the discussion in [1], phy_set_attrs/phy_get_attrs APIs in
>>> future could maybe used by other drivers replacing existing individual
>>> functions for attributes bus_width and mode which are
>>> phy_set_bus_width/phy_get_bus_width and
>> phy_set_mode/phy_get_mode. So
>>> this usage in Torrent PHY driver is an example implementation of the API.
>>>
>>> [1]
>>> https://urldefense.com/v3/__https://lkml.org/lkml/2020/5/18/472__;!!EH
>>> scmS1ygiU1lA!QKTTI7BS1R35a_zoMfJsY4A4yCtEKrQNtiAXTyIZ-
>> SYIEEibYdpBMJTll
>>> Yrd-00$
>>
>> This doesn't seem a very good API to me :-S It will require callers to always
>> call phy_get_attrs() first, modify the attributes they want to set, and then call
>> phy_set_attrs(). Not only will be copy the whole phy_attrs structure
>> needlessly, it will also not be an atomic operation as someone else could
>> modify attributes between the get and set calls.
>> The lack of atomicity may not be an issue in practice if there's a single user of
>> the PHY at all times, but in that case no mutex is needed.

What if the consumer tries to set an attribute at the middle of a
phy_power_on() operation? That is still a valid operation and phy core layer
should try to prevent it no?
>>
>> I think this series tries to fix a problem that doesn't exist.
> 
> Thanks Laurent for your comments.
> 
> Hi Kishon,
> 
> Could you please suggest what would be the better approach regarding this PHY
> attributes series. Should we add individual get/set functions for new attribute
> max_link_rate just like mode and bus_width, or should we use phy_get_attrs()
> and phy_set_attrs() functions removing mutex.  Your suggestions would really help.

I think Laurent's point is not having an API at all for configuring attributes
and access them manually?

Thanks
Kishon

> 
> Thanks & regards,
> Swapnil
> 
>>
>>>>>  		} else {
>>>>>  			dev_err(dev, "Driver supports only
>> PHY_TYPE_DP\n");
>>>>>  			ret = -ENOTSUPP;
>>
>> --
>> Regards,
>>
>> Laurent Pinchart

Re: [PATCH v5 2/2] phy: cadence-torrent: Use kernel PHY API to set PHY attributes

[Laurent Pinchart]

Hi Kishon,

On Thu, Sep 03, 2020 at 05:00:14PM +0530, Kishon Vijay Abraham I wrote:
> On 9/3/2020 4:29 PM, Swapnil Kashinath Jakhade wrote:
> > On Wednesday, September 2, 2020 5:47 PM, Laurent Pinchart wrote:
> >> On Wed, Sep 02, 2020 at 07:09:21AM +0000, Swapnil Kashinath Jakhade wrote:
> >>> On Wednesday, September 2, 2020 6:00 AM Laurent Pinchart wrote:
> >>>> On Mon, Aug 24, 2020 at 08:28:31PM +0200, Swapnil Jakhade wrote:
> >>>>> Use generic PHY framework function phy_set_attrs() to set number
> >>>>> of lanes and maximum link rate supported by PHY.
> >>>>>
> >>>>> Signed-off-by: Swapnil Jakhade <sjakhade@cadence.com>
> >>>>> Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
> >>>>> ---
> >>>>>  drivers/phy/cadence/phy-cadence-torrent.c | 7 +++++++
> >>>>>  1 file changed, 7 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/phy/cadence/phy-cadence-torrent.c
> >>>>> b/drivers/phy/cadence/phy-cadence-torrent.c
> >>>>> index 7116127358ee..eca71467c4a8 100644
> >>>>> --- a/drivers/phy/cadence/phy-cadence-torrent.c
> >>>>> +++ b/drivers/phy/cadence/phy-cadence-torrent.c
> >>>>> @@ -1710,6 +1710,7 @@ static int cdns_torrent_phy_probe(struct platform_device *pdev)
> >>>>>  	struct cdns_torrent_phy *cdns_phy;
> >>>>>  	struct device *dev = &pdev->dev;
> >>>>>  	struct phy_provider *phy_provider;
> >>>>> +	struct phy_attrs torrent_attr;
> >>>>>  	const struct of_device_id *match;
> >>>>>  	struct cdns_torrent_data *data;
> >>>>>  	struct device_node *child;
> >>>>> @@ -1852,6 +1853,12 @@ static int cdns_torrent_phy_probe(struct platform_device *pdev)
> >>>>>  				 cdns_phy->phys[node].num_lanes,
> >>>>>  				 cdns_phy->max_bit_rate / 1000,
> >>>>>  				 cdns_phy->max_bit_rate % 1000);
> >>>>> +
> >>>>> +			torrent_attr.bus_width = cdns_phy->phys[node].num_lanes;
> >>>>> +			torrent_attr.max_link_rate = cdns_phy->max_bit_rate;
> >>>>> +			torrent_attr.mode = PHY_MODE_DP;
> >>>>> +
> >>>>> +			phy_set_attrs(gphy, &torrent_attr);
> >>>>
> >>>> Why is this better than accessing the attributes manually as follows ?
> >>>>
> >>>> 			gphy->attrs.bus_width = cdns_phy->phys[node].num_lanes;
> >>>> 			gphy->attrs.max_link_rate = cdns_phy->max_bit_rate;
> >>>> 			gphy->attrs.mode = PHY_MODE_DP;
> >>>>
> >>>> This is called in cdns_torrent_phy_probe(), before the PHY provider
> >>>> is registered, so nothing can access the PHY yet. What race
> >>>> condition are you trying to protect against with usage of phy_set_attrs() ?
> >>>
> >>> I agree that for Cadence DP bridge driver and Torrent PHY driver use
> >>> case, it would not matter even if we set the attributes in Torrent PHY
> >>> driver in a way you suggested above.
> >>> But as per the discussion in [1], phy_set_attrs/phy_get_attrs APIs in
> >>> future could maybe used by other drivers replacing existing individual
> >>> functions for attributes bus_width and mode which are
> >>> phy_set_bus_width/phy_get_bus_width and phy_set_mode/phy_get_mode. So
> >>> this usage in Torrent PHY driver is an example implementation of the API.
> >>>
> >>> [1]
> >>> https://urldefense.com/v3/__https://lkml.org/lkml/2020/5/18/472__;!!EH
> >>> scmS1ygiU1lA!QKTTI7BS1R35a_zoMfJsY4A4yCtEKrQNtiAXTyIZ-SYIEEibYdpBMJTll
> >>> Yrd-00$
> >>
> >> This doesn't seem a very good API to me :-S It will require callers to always
> >> call phy_get_attrs() first, modify the attributes they want to set, and then call
> >> phy_set_attrs(). Not only will be copy the whole phy_attrs structure
> >> needlessly, it will also not be an atomic operation as someone else could
> >> modify attributes between the get and set calls.
> >> The lack of atomicity may not be an issue in practice if there's a single user of
> >> the PHY at all times, but in that case no mutex is needed.
> 
> What if the consumer tries to set an attribute at the middle of a
> phy_power_on() operation? That is still a valid operation and phy core layer
> should try to prevent it no?

I see multiple questions here.

First of all, unless I'm mistaken, the attributes set here are static
properties, set by the PHY driver at probe time, and only read by PHY
consumers. There should be no need for any kind of protection or special
API to access them.

Then, there's the question of how to handle dynamic attributes. In
theory a dynamic attribute could be changed at any time, and thus race
wit, for instance phy_power_on(). However, the proposed API won't help
much address this issue. Using a mutex will indeed ensure that the
attribute change will be serialized with other operations, but it won't
give any guarantee to the PHY consumer on whether the attribute will be
set before or after phy_power_on() is processed. The consumer will not
know if the new value of the attribute has been taken into account.

The question is thus whether we want to make the PHY consumer API
thread-safe (note that due to the usage of a mutex, we don't support
calling most of the API functions from an interrupt handler, so it
really requires the consumer to use a work queue, a thread, or possibly
a threaded interrupt). If the answer is yes, the API should define what
use cases are valid, and how the PHY has to behave. This includes
documenting when new attribute values can be set, and when they are
taken into account. If we had to document this as part of this patch
series, we would have to state that the new values are taken into
account at an undefined point of time if the attribute set call is
concurrent with other API calls, which makes the API ill-defined in my
opinion. I expect that we would need to turn attribute setting into a
callback to the PHY driver in that case, or at least make it a more
complex operation handled by the PHY core that would use the existing
PHY ops to reconfigure the PHY.

Is it worth it allowing drivers to call the PHY API from different
threads as opposed to requiring consumers to serialize calls if their
use cases require so ? I would expect most consumers to only try to
reconfigure a PHY when it's stopped, or to manually stop, reconfigure
and restart the PHY.

> >> I think this series tries to fix a problem that doesn't exist.
> > 
> > Thanks Laurent for your comments.
> > 
> > Hi Kishon,
> > 
> > Could you please suggest what would be the better approach regarding this PHY
> > attributes series. Should we add individual get/set functions for new attribute
> > max_link_rate just like mode and bus_width, or should we use phy_get_attrs()
> > and phy_set_attrs() functions removing mutex.  Your suggestions would really help.
> 
> I think Laurent's point is not having an API at all for configuring attributes
> and access them manually?

If the answer to the above question is that a thread-safe API isn't
worth it as we wouldn't have good use cases for it, then I think
accessing the attributes manually is all we need.

> >>>>>  		} else {
> >>>>>  			dev_err(dev, "Driver supports only PHY_TYPE_DP\n");
> >>>>>  			ret = -ENOTSUPP;

-- 
Regards,

Laurent Pinchart

RE: [PATCH v5 2/2] phy: cadence-torrent: Use kernel PHY API to set PHY attributes

[Milind Parab]

Hi Kishon,

>-----Original Message-----
>From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>Sent: Thursday, September 3, 2020 9:00 PM
>To: Kishon Vijay Abraham I <kishon@ti.com>
>Cc: Swapnil Kashinath Jakhade <sjakhade@cadence.com>; vkoul@kernel.org;
>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 2/2] phy: cadence-torrent: Use kernel PHY API to set
>PHY attributes
>
>EXTERNAL MAIL
>
>
>Hi Kishon,
>
>On Thu, Sep 03, 2020 at 05:00:14PM +0530, Kishon Vijay Abraham I wrote:
>> On 9/3/2020 4:29 PM, Swapnil Kashinath Jakhade wrote:
>> > On Wednesday, September 2, 2020 5:47 PM, Laurent Pinchart wrote:
>> >> On Wed, Sep 02, 2020 at 07:09:21AM +0000, Swapnil Kashinath Jakhade
>wrote:
>> >>> On Wednesday, September 2, 2020 6:00 AM Laurent Pinchart wrote:
>> >>>> On Mon, Aug 24, 2020 at 08:28:31PM +0200, Swapnil Jakhade wrote:
>> >>>>> Use generic PHY framework function phy_set_attrs() to set number
>> >>>>> of lanes and maximum link rate supported by PHY.
>> >>>>>
>> >>>>> Signed-off-by: Swapnil Jakhade <sjakhade@cadence.com>
>> >>>>> Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
>> >>>>> ---
>> >>>>>  drivers/phy/cadence/phy-cadence-torrent.c | 7 +++++++
>> >>>>>  1 file changed, 7 insertions(+)
>> >>>>>
>> >>>>> diff --git a/drivers/phy/cadence/phy-cadence-torrent.c
>> >>>>> b/drivers/phy/cadence/phy-cadence-torrent.c
>> >>>>> index 7116127358ee..eca71467c4a8 100644
>> >>>>> --- a/drivers/phy/cadence/phy-cadence-torrent.c
>> >>>>> +++ b/drivers/phy/cadence/phy-cadence-torrent.c
>> >>>>> @@ -1710,6 +1710,7 @@ static int cdns_torrent_phy_probe(struct
>platform_device *pdev)
>> >>>>>  	struct cdns_torrent_phy *cdns_phy;
>> >>>>>  	struct device *dev = &pdev->dev;
>> >>>>>  	struct phy_provider *phy_provider;
>> >>>>> +	struct phy_attrs torrent_attr;
>> >>>>>  	const struct of_device_id *match;
>> >>>>>  	struct cdns_torrent_data *data;
>> >>>>>  	struct device_node *child;
>> >>>>> @@ -1852,6 +1853,12 @@ static int cdns_torrent_phy_probe(struct
>platform_device *pdev)
>> >>>>>  				 cdns_phy->phys[node].num_lanes,
>> >>>>>  				 cdns_phy->max_bit_rate / 1000,
>> >>>>>  				 cdns_phy->max_bit_rate % 1000);
>> >>>>> +
>> >>>>> +			torrent_attr.bus_width = cdns_phy-
>>phys[node].num_lanes;
>> >>>>> +			torrent_attr.max_link_rate = cdns_phy-
>>max_bit_rate;
>> >>>>> +			torrent_attr.mode = PHY_MODE_DP;
>> >>>>> +
>> >>>>> +			phy_set_attrs(gphy, &torrent_attr);
>> >>>>
>> >>>> Why is this better than accessing the attributes manually as follows ?
>> >>>>
>> >>>> 			gphy->attrs.bus_width = cdns_phy-
>>phys[node].num_lanes;
>> >>>> 			gphy->attrs.max_link_rate = cdns_phy-
>>max_bit_rate;
>> >>>> 			gphy->attrs.mode = PHY_MODE_DP;
>> >>>>
>> >>>> This is called in cdns_torrent_phy_probe(), before the PHY
>> >>>> provider is registered, so nothing can access the PHY yet. What
>> >>>> race condition are you trying to protect against with usage of
>phy_set_attrs() ?
>> >>>
>> >>> I agree that for Cadence DP bridge driver and Torrent PHY driver
>> >>> use case, it would not matter even if we set the attributes in
>> >>> Torrent PHY driver in a way you suggested above.
>> >>> But as per the discussion in [1], phy_set_attrs/phy_get_attrs APIs
>> >>> in future could maybe used by other drivers replacing existing
>> >>> individual functions for attributes bus_width and mode which are
>> >>> phy_set_bus_width/phy_get_bus_width and
>phy_set_mode/phy_get_mode.
>> >>> So this usage in Torrent PHY driver is an example implementation of the
>API.
>> >>>
>> >>> [1]
>> >>> https://urldefense.com/v3/__https://lkml.org/lkml/2020/5/18/472__;
>> >>> !!EH
>> >>> scmS1ygiU1lA!QKTTI7BS1R35a_zoMfJsY4A4yCtEKrQNtiAXTyIZ-
>SYIEEibYdpBM
>> >>> JTll
>> >>> Yrd-00$
>> >>
>> >> This doesn't seem a very good API to me :-S It will require callers
>> >> to always call phy_get_attrs() first, modify the attributes they
>> >> want to set, and then call phy_set_attrs(). Not only will be copy
>> >> the whole phy_attrs structure needlessly, it will also not be an
>> >> atomic operation as someone else could modify attributes between the
>get and set calls.
>> >> The lack of atomicity may not be an issue in practice if there's a
>> >> single user of the PHY at all times, but in that case no mutex is needed.
>>
>> What if the consumer tries to set an attribute at the middle of a
>> phy_power_on() operation? That is still a valid operation and phy core
>> layer should try to prevent it no?
>
>I see multiple questions here.
>
>First of all, unless I'm mistaken, the attributes set here are static properties,
>set by the PHY driver at probe time, and only read by PHY consumers. There
>should be no need for any kind of protection or special API to access them.
>
>Then, there's the question of how to handle dynamic attributes. In theory a
>dynamic attribute could be changed at any time, and thus race wit, for
>instance phy_power_on(). However, the proposed API won't help much
>address this issue. Using a mutex will indeed ensure that the attribute change
>will be serialized with other operations, but it won't give any guarantee to the
>PHY consumer on whether the attribute will be set before or after
>phy_power_on() is processed. The consumer will not know if the new value
>of the attribute has been taken into account.
>
>The question is thus whether we want to make the PHY consumer API thread-
>safe (note that due to the usage of a mutex, we don't support calling most of
>the API functions from an interrupt handler, so it really requires the consumer
>to use a work queue, a thread, or possibly a threaded interrupt). If the answer
>is yes, the API should define what use cases are valid, and how the PHY has to
>behave. This includes documenting when new attribute values can be set, and
>when they are taken into account. If we had to document this as part of this
>patch series, we would have to state that the new values are taken into
>account at an undefined point of time if the attribute set call is concurrent
>with other API calls, which makes the API ill-defined in my opinion. I expect
>that we would need to turn attribute setting into a callback to the PHY driver
>in that case, or at least make it a more complex operation handled by the PHY
>core that would use the existing PHY ops to reconfigure the PHY.
>
>Is it worth it allowing drivers to call the PHY API from different threads as
>opposed to requiring consumers to serialize calls if their use cases require so ?
>I would expect most consumers to only try to reconfigure a PHY when it's
>stopped, or to manually stop, reconfigure and restart the PHY.
>
>> >> I think this series tries to fix a problem that doesn't exist.
>> >
>> > Thanks Laurent for your comments.
>> >
>> > Hi Kishon,
>> >
>> > Could you please suggest what would be the better approach regarding
>> > this PHY attributes series. Should we add individual get/set
>> > functions for new attribute max_link_rate just like mode and
>> > bus_width, or should we use phy_get_attrs() and phy_set_attrs()
>functions removing mutex.  Your suggestions would really help.
>>
>> I think Laurent's point is not having an API at all for configuring
>> attributes and access them manually?
>
>If the answer to the above question is that a thread-safe API isn't worth it as
>we wouldn't have good use cases for it, then I think accessing the attributes
>manually is all we need.
>

Should we proceed accessing attribute manually

>> >>>>>  		} else {
>> >>>>>  			dev_err(dev, "Driver supports only
>PHY_TYPE_DP\n");
>> >>>>>  			ret = -ENOTSUPP;
>
>--
>Regards,
>
>Laurent Pinchart

Re: [PATCH v5 2/2] phy: cadence-torrent: Use kernel PHY API to set PHY attributes

[Kishon Vijay Abraham I]

Hi Milind,

On 08/09/20 7:45 pm, Milind Parab wrote:
> Hi Kishon,
> 
>> -----Original Message-----
>> From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Sent: Thursday, September 3, 2020 9:00 PM
>> To: Kishon Vijay Abraham I <kishon@ti.com>
>> Cc: Swapnil Kashinath Jakhade <sjakhade@cadence.com>; vkoul@kernel.org;
>> 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 2/2] phy: cadence-torrent: Use kernel PHY API to set
>> PHY attributes
>>
>> EXTERNAL MAIL
>>
>>
>> Hi Kishon,
>>
>> On Thu, Sep 03, 2020 at 05:00:14PM +0530, Kishon Vijay Abraham I wrote:
>>> On 9/3/2020 4:29 PM, Swapnil Kashinath Jakhade wrote:
>>>> On Wednesday, September 2, 2020 5:47 PM, Laurent Pinchart wrote:
>>>>> On Wed, Sep 02, 2020 at 07:09:21AM +0000, Swapnil Kashinath Jakhade
>> wrote:
>>>>>> On Wednesday, September 2, 2020 6:00 AM Laurent Pinchart wrote:
>>>>>>> On Mon, Aug 24, 2020 at 08:28:31PM +0200, Swapnil Jakhade wrote:
>>>>>>>> Use generic PHY framework function phy_set_attrs() to set number
>>>>>>>> of lanes and maximum link rate supported by PHY.
>>>>>>>>
>>>>>>>> Signed-off-by: Swapnil Jakhade <sjakhade@cadence.com>
>>>>>>>> Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
>>>>>>>> ---
>>>>>>>>   drivers/phy/cadence/phy-cadence-torrent.c | 7 +++++++
>>>>>>>>   1 file changed, 7 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/phy/cadence/phy-cadence-torrent.c
>>>>>>>> b/drivers/phy/cadence/phy-cadence-torrent.c
>>>>>>>> index 7116127358ee..eca71467c4a8 100644
>>>>>>>> --- a/drivers/phy/cadence/phy-cadence-torrent.c
>>>>>>>> +++ b/drivers/phy/cadence/phy-cadence-torrent.c
>>>>>>>> @@ -1710,6 +1710,7 @@ static int cdns_torrent_phy_probe(struct
>> platform_device *pdev)
>>>>>>>>   	struct cdns_torrent_phy *cdns_phy;
>>>>>>>>   	struct device *dev = &pdev->dev;
>>>>>>>>   	struct phy_provider *phy_provider;
>>>>>>>> +	struct phy_attrs torrent_attr;
>>>>>>>>   	const struct of_device_id *match;
>>>>>>>>   	struct cdns_torrent_data *data;
>>>>>>>>   	struct device_node *child;
>>>>>>>> @@ -1852,6 +1853,12 @@ static int cdns_torrent_phy_probe(struct
>> platform_device *pdev)
>>>>>>>>   				 cdns_phy->phys[node].num_lanes,
>>>>>>>>   				 cdns_phy->max_bit_rate / 1000,
>>>>>>>>   				 cdns_phy->max_bit_rate % 1000);
>>>>>>>> +
>>>>>>>> +			torrent_attr.bus_width = cdns_phy-
>>> phys[node].num_lanes;
>>>>>>>> +			torrent_attr.max_link_rate = cdns_phy-
>>> max_bit_rate;
>>>>>>>> +			torrent_attr.mode = PHY_MODE_DP;
>>>>>>>> +
>>>>>>>> +			phy_set_attrs(gphy, &torrent_attr);
>>>>>>>
>>>>>>> Why is this better than accessing the attributes manually as follows ?
>>>>>>>
>>>>>>> 			gphy->attrs.bus_width = cdns_phy-
>>> phys[node].num_lanes;
>>>>>>> 			gphy->attrs.max_link_rate = cdns_phy-
>>> max_bit_rate;
>>>>>>> 			gphy->attrs.mode = PHY_MODE_DP;
>>>>>>>
>>>>>>> This is called in cdns_torrent_phy_probe(), before the PHY
>>>>>>> provider is registered, so nothing can access the PHY yet. What
>>>>>>> race condition are you trying to protect against with usage of
>> phy_set_attrs() ?
>>>>>>
>>>>>> I agree that for Cadence DP bridge driver and Torrent PHY driver
>>>>>> use case, it would not matter even if we set the attributes in
>>>>>> Torrent PHY driver in a way you suggested above.
>>>>>> But as per the discussion in [1], phy_set_attrs/phy_get_attrs APIs
>>>>>> in future could maybe used by other drivers replacing existing
>>>>>> individual functions for attributes bus_width and mode which are
>>>>>> phy_set_bus_width/phy_get_bus_width and
>> phy_set_mode/phy_get_mode.
>>>>>> So this usage in Torrent PHY driver is an example implementation of the
>> API.
>>>>>>
>>>>>> [1]
>>>>>> https://urldefense.com/v3/__https://lkml.org/lkml/2020/5/18/472__;
>>>>>> !!EH
>>>>>> scmS1ygiU1lA!QKTTI7BS1R35a_zoMfJsY4A4yCtEKrQNtiAXTyIZ-
>> SYIEEibYdpBM
>>>>>> JTll
>>>>>> Yrd-00$
>>>>>
>>>>> This doesn't seem a very good API to me :-S It will require callers
>>>>> to always call phy_get_attrs() first, modify the attributes they
>>>>> want to set, and then call phy_set_attrs(). Not only will be copy
>>>>> the whole phy_attrs structure needlessly, it will also not be an
>>>>> atomic operation as someone else could modify attributes between the
>> get and set calls.
>>>>> The lack of atomicity may not be an issue in practice if there's a
>>>>> single user of the PHY at all times, but in that case no mutex is needed.
>>>
>>> What if the consumer tries to set an attribute at the middle of a
>>> phy_power_on() operation? That is still a valid operation and phy core
>>> layer should try to prevent it no?
>>
>> I see multiple questions here.
>>
>> First of all, unless I'm mistaken, the attributes set here are static properties,
>> set by the PHY driver at probe time, and only read by PHY consumers. There
>> should be no need for any kind of protection or special API to access them.
>>
>> Then, there's the question of how to handle dynamic attributes. In theory a
>> dynamic attribute could be changed at any time, and thus race wit, for
>> instance phy_power_on(). However, the proposed API won't help much
>> address this issue. Using a mutex will indeed ensure that the attribute change
>> will be serialized with other operations, but it won't give any guarantee to the
>> PHY consumer on whether the attribute will be set before or after
>> phy_power_on() is processed. The consumer will not know if the new value
>> of the attribute has been taken into account.
>>
>> The question is thus whether we want to make the PHY consumer API thread-
>> safe (note that due to the usage of a mutex, we don't support calling most of
>> the API functions from an interrupt handler, so it really requires the consumer
>> to use a work queue, a thread, or possibly a threaded interrupt). If the answer
>> is yes, the API should define what use cases are valid, and how the PHY has to
>> behave. This includes documenting when new attribute values can be set, and
>> when they are taken into account. If we had to document this as part of this
>> patch series, we would have to state that the new values are taken into
>> account at an undefined point of time if the attribute set call is concurrent
>> with other API calls, which makes the API ill-defined in my opinion. I expect
>> that we would need to turn attribute setting into a callback to the PHY driver
>> in that case, or at least make it a more complex operation handled by the PHY
>> core that would use the existing PHY ops to reconfigure the PHY.
>>
>> Is it worth it allowing drivers to call the PHY API from different threads as
>> opposed to requiring consumers to serialize calls if their use cases require so ?
>> I would expect most consumers to only try to reconfigure a PHY when it's
>> stopped, or to manually stop, reconfigure and restart the PHY.
>>
>>>>> I think this series tries to fix a problem that doesn't exist.
>>>>
>>>> Thanks Laurent for your comments.
>>>>
>>>> Hi Kishon,
>>>>
>>>> Could you please suggest what would be the better approach regarding
>>>> this PHY attributes series. Should we add individual get/set
>>>> functions for new attribute max_link_rate just like mode and
>>>> bus_width, or should we use phy_get_attrs() and phy_set_attrs()
>> functions removing mutex.  Your suggestions would really help.
>>>
>>> I think Laurent's point is not having an API at all for configuring
>>> attributes and access them manually?
>>
>> If the answer to the above question is that a thread-safe API isn't worth it as
>> we wouldn't have good use cases for it, then I think accessing the attributes
>> manually is all we need.
>>
> 
> Should we proceed accessing attribute manually

yeah, let's deal with dynamic attributes later when the use cases arise 
unless Vinod disagrees.

Thanks
Kishon

Re: [PATCH v5 2/2] phy: cadence-torrent: Use kernel PHY API to set PHY attributes

[Vinod Koul]

On 10-09-20, 11:45, Kishon Vijay Abraham I wrote:
> Hi Milind,
> 
> On 08/09/20 7:45 pm, Milind Parab wrote:
> > Hi Kishon,
> > 
> > > -----Original Message-----
> > > From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Sent: Thursday, September 3, 2020 9:00 PM
> > > To: Kishon Vijay Abraham I <kishon@ti.com>
> > > Cc: Swapnil Kashinath Jakhade <sjakhade@cadence.com>; vkoul@kernel.org;
> > > 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 2/2] phy: cadence-torrent: Use kernel PHY API to set
> > > PHY attributes
> > > 
> > > EXTERNAL MAIL
> > > 
> > > 
> > > Hi Kishon,
> > > 
> > > On Thu, Sep 03, 2020 at 05:00:14PM +0530, Kishon Vijay Abraham I wrote:
> > > > On 9/3/2020 4:29 PM, Swapnil Kashinath Jakhade wrote:
> > > > > On Wednesday, September 2, 2020 5:47 PM, Laurent Pinchart wrote:
> > > > > > On Wed, Sep 02, 2020 at 07:09:21AM +0000, Swapnil Kashinath Jakhade
> > > wrote:
> > > > > > > On Wednesday, September 2, 2020 6:00 AM Laurent Pinchart wrote:
> > > > > > > > On Mon, Aug 24, 2020 at 08:28:31PM +0200, Swapnil Jakhade wrote:
> > > > > > > > > Use generic PHY framework function phy_set_attrs() to set number
> > > > > > > > > of lanes and maximum link rate supported by PHY.
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Swapnil Jakhade <sjakhade@cadence.com>
> > > > > > > > > Acked-by: Kishon Vijay Abraham I <kishon@ti.com>
> > > > > > > > > ---
> > > > > > > > >   drivers/phy/cadence/phy-cadence-torrent.c | 7 +++++++
> > > > > > > > >   1 file changed, 7 insertions(+)
> > > > > > > > > 
> > > > > > > > > diff --git a/drivers/phy/cadence/phy-cadence-torrent.c
> > > > > > > > > b/drivers/phy/cadence/phy-cadence-torrent.c
> > > > > > > > > index 7116127358ee..eca71467c4a8 100644
> > > > > > > > > --- a/drivers/phy/cadence/phy-cadence-torrent.c
> > > > > > > > > +++ b/drivers/phy/cadence/phy-cadence-torrent.c
> > > > > > > > > @@ -1710,6 +1710,7 @@ static int cdns_torrent_phy_probe(struct
> > > platform_device *pdev)
> > > > > > > > >   	struct cdns_torrent_phy *cdns_phy;
> > > > > > > > >   	struct device *dev = &pdev->dev;
> > > > > > > > >   	struct phy_provider *phy_provider;
> > > > > > > > > +	struct phy_attrs torrent_attr;
> > > > > > > > >   	const struct of_device_id *match;
> > > > > > > > >   	struct cdns_torrent_data *data;
> > > > > > > > >   	struct device_node *child;
> > > > > > > > > @@ -1852,6 +1853,12 @@ static int cdns_torrent_phy_probe(struct
> > > platform_device *pdev)
> > > > > > > > >   				 cdns_phy->phys[node].num_lanes,
> > > > > > > > >   				 cdns_phy->max_bit_rate / 1000,
> > > > > > > > >   				 cdns_phy->max_bit_rate % 1000);
> > > > > > > > > +
> > > > > > > > > +			torrent_attr.bus_width = cdns_phy-
> > > > phys[node].num_lanes;
> > > > > > > > > +			torrent_attr.max_link_rate = cdns_phy-
> > > > max_bit_rate;
> > > > > > > > > +			torrent_attr.mode = PHY_MODE_DP;
> > > > > > > > > +
> > > > > > > > > +			phy_set_attrs(gphy, &torrent_attr);
> > > > > > > > 
> > > > > > > > Why is this better than accessing the attributes manually as follows ?
> > > > > > > > 
> > > > > > > > 			gphy->attrs.bus_width = cdns_phy-
> > > > phys[node].num_lanes;
> > > > > > > > 			gphy->attrs.max_link_rate = cdns_phy-
> > > > max_bit_rate;
> > > > > > > > 			gphy->attrs.mode = PHY_MODE_DP;
> > > > > > > > 
> > > > > > > > This is called in cdns_torrent_phy_probe(), before the PHY
> > > > > > > > provider is registered, so nothing can access the PHY yet. What
> > > > > > > > race condition are you trying to protect against with usage of
> > > phy_set_attrs() ?
> > > > > > > 
> > > > > > > I agree that for Cadence DP bridge driver and Torrent PHY driver
> > > > > > > use case, it would not matter even if we set the attributes in
> > > > > > > Torrent PHY driver in a way you suggested above.
> > > > > > > But as per the discussion in [1], phy_set_attrs/phy_get_attrs APIs
> > > > > > > in future could maybe used by other drivers replacing existing
> > > > > > > individual functions for attributes bus_width and mode which are
> > > > > > > phy_set_bus_width/phy_get_bus_width and
> > > phy_set_mode/phy_get_mode.
> > > > > > > So this usage in Torrent PHY driver is an example implementation of the
> > > API.
> > > > > > > 
> > > > > > > [1]
> > > > > > > https://urldefense.com/v3/__https://lkml.org/lkml/2020/5/18/472__;
> > > > > > > !!EH
> > > > > > > scmS1ygiU1lA!QKTTI7BS1R35a_zoMfJsY4A4yCtEKrQNtiAXTyIZ-
> > > SYIEEibYdpBM
> > > > > > > JTll
> > > > > > > Yrd-00$
> > > > > > 
> > > > > > This doesn't seem a very good API to me :-S It will require callers
> > > > > > to always call phy_get_attrs() first, modify the attributes they
> > > > > > want to set, and then call phy_set_attrs(). Not only will be copy
> > > > > > the whole phy_attrs structure needlessly, it will also not be an
> > > > > > atomic operation as someone else could modify attributes between the
> > > get and set calls.
> > > > > > The lack of atomicity may not be an issue in practice if there's a
> > > > > > single user of the PHY at all times, but in that case no mutex is needed.
> > > > 
> > > > What if the consumer tries to set an attribute at the middle of a
> > > > phy_power_on() operation? That is still a valid operation and phy core
> > > > layer should try to prevent it no?
> > > 
> > > I see multiple questions here.
> > > 
> > > First of all, unless I'm mistaken, the attributes set here are static properties,
> > > set by the PHY driver at probe time, and only read by PHY consumers. There
> > > should be no need for any kind of protection or special API to access them.
> > > 
> > > Then, there's the question of how to handle dynamic attributes. In theory a
> > > dynamic attribute could be changed at any time, and thus race wit, for
> > > instance phy_power_on(). However, the proposed API won't help much
> > > address this issue. Using a mutex will indeed ensure that the attribute change
> > > will be serialized with other operations, but it won't give any guarantee to the
> > > PHY consumer on whether the attribute will be set before or after
> > > phy_power_on() is processed. The consumer will not know if the new value
> > > of the attribute has been taken into account.
> > > 
> > > The question is thus whether we want to make the PHY consumer API thread-
> > > safe (note that due to the usage of a mutex, we don't support calling most of
> > > the API functions from an interrupt handler, so it really requires the consumer
> > > to use a work queue, a thread, or possibly a threaded interrupt). If the answer
> > > is yes, the API should define what use cases are valid, and how the PHY has to
> > > behave. This includes documenting when new attribute values can be set, and
> > > when they are taken into account. If we had to document this as part of this
> > > patch series, we would have to state that the new values are taken into
> > > account at an undefined point of time if the attribute set call is concurrent
> > > with other API calls, which makes the API ill-defined in my opinion. I expect
> > > that we would need to turn attribute setting into a callback to the PHY driver
> > > in that case, or at least make it a more complex operation handled by the PHY
> > > core that would use the existing PHY ops to reconfigure the PHY.
> > > 
> > > Is it worth it allowing drivers to call the PHY API from different threads as
> > > opposed to requiring consumers to serialize calls if their use cases require so ?
> > > I would expect most consumers to only try to reconfigure a PHY when it's
> > > stopped, or to manually stop, reconfigure and restart the PHY.
> > > 
> > > > > > I think this series tries to fix a problem that doesn't exist.
> > > > > 
> > > > > Thanks Laurent for your comments.
> > > > > 
> > > > > Hi Kishon,
> > > > > 
> > > > > Could you please suggest what would be the better approach regarding
> > > > > this PHY attributes series. Should we add individual get/set
> > > > > functions for new attribute max_link_rate just like mode and
> > > > > bus_width, or should we use phy_get_attrs() and phy_set_attrs()
> > > functions removing mutex.  Your suggestions would really help.
> > > > 
> > > > I think Laurent's point is not having an API at all for configuring
> > > > attributes and access them manually?
> > > 
> > > If the answer to the above question is that a thread-safe API isn't worth it as
> > > we wouldn't have good use cases for it, then I think accessing the attributes
> > > manually is all we need.
> > > 
> > 
> > Should we proceed accessing attribute manually
> 
> yeah, let's deal with dynamic attributes later when the use cases arise
> unless Vinod disagrees.

I am fine with it, we can modify later as the need arises

-- 
~Vinod