Hi Laurent, Thank you for your review comments.
> -----Original Message----- > From: Laurent Pinchart <laurent.pinch...@ideasonboard.com> > Sent: Wednesday, September 2, 2020 6:00 AM > To: Swapnil Kashinath Jakhade <sjakh...@cadence.com> > Cc: vk...@kernel.org; kis...@ti.com; linux-kernel@vger.kernel.org; > max...@cerno.tech; Milind Parab <mpa...@cadence.com>; Yuti Suresh > Amonkar <yamon...@cadence.com>; nsek...@ti.com; > tomi.valkei...@ti.com; jsa...@ti.com; prane...@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 <sjakh...@cadence.com> > > Acked-by: Kishon Vijay Abraham I <kis...@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
