> -----Original Message-----
> From: Laurent Pinchart <[email protected]>
> Sent: Wednesday, September 2, 2020 5:47 PM
> To: Swapnil Kashinath Jakhade <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; Milind Parab <[email protected]>; Yuti Suresh
> Amonkar <[email protected]>; [email protected];
> [email protected]; [email protected]; [email protected]
> 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 <[email protected]>
> > > > Acked-by: Kishon Vijay Abraham I <[email protected]>
> > > > ---
> > > > 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
