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@xxxxxxxxxxxxxxxx>
> Sent: Wednesday, September 2, 2020 6:00 AM
> To: Swapnil Kashinath Jakhade <sjakhade@xxxxxxxxxxx>
> Cc: vkoul@xxxxxxxxxx; kishon@xxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> maxime@xxxxxxxxxx; Milind Parab <mparab@xxxxxxxxxxx>; Yuti Suresh
> Amonkar <yamonkar@xxxxxxxxxxx>; nsekhar@xxxxxx;
> tomi.valkeinen@xxxxxx; jsarha@xxxxxx; praneeth@xxxxxx
> 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@xxxxxxxxxxx>
> > Acked-by: Kishon Vijay Abraham I <kishon@xxxxxx>
> > ---
> >  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