Hi,
Thanks for your comments.
> -----Original Message-----
> From: Kishon Vijay Abraham I <kis...@ti.com>
> Sent: Monday, July 13, 2020 5:28 PM
> To: Vinod Koul <vk...@kernel.org>; Swapnil Kashinath Jakhade
> <sjakh...@cadence.com>
> Cc: 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 v3 1/2] phy: Add new PHY attribute max_link_rate and
> APIs to get/set PHY attributes
>
> EXTERNAL MAIL
>
>
>
>
> On 7/13/2020 4:41 PM, Vinod Koul wrote:
> > On 13-07-20, 11:38, 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_set_attrs() to set attribute values in the PHY provider driver.
> >> Use phy_get_attrs() to get attribute values in the controller driver.
> >>
> >> Signed-off-by: Yuti Amonkar <yamon...@cadence.com>
> >> Signed-off-by: Swapnil Jakhade <sjakh...@cadence.com>
> >> ---
> >> include/linux/phy/phy.h | 22 ++++++++++++++++++++++
> >> 1 file changed, 22 insertions(+)
> >>
> >> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h index
> >> bcee8eba62b3..7fb59359ab7b 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,16 @@ static inline void phy_set_bus_width(struct phy
> >> *phy, int bus_width) {
> >> phy->attrs.bus_width = bus_width;
> >> }
> >> +
> >> +static inline void phy_get_attrs(struct phy *phy, struct phy_attrs
> >> +*attrs) {
> >> + memcpy(attrs, &phy->attrs, sizeof(struct phy_attrs)); }
> >> +
> >> +static inline void phy_set_attrs(struct phy *phy, struct phy_attrs
> >> +attrs) {
> >> + memcpy(&phy->attrs, &attrs, sizeof(struct phy_attrs)); }
> >
> > we already have APIs for mode and bus_width so why not add one for
> > link_rate and call them?
First version of this patch series [1] was based on this approach. But as per
the further discussion in the same thread [2], phy_get_attrs/phy_set_attrs are
implemented.
[1] https://lkml.org/lkml/2020/4/28/139
[2] https://lkml.org/lkml/2020/5/18/472
> >
> > Also I see you are using phy_set_attrs() in second patch, why add
> > phy_get_attrs() when we have no user?
As mentioned in cover letter, phy_get_attrs() is planned to be used in Cadence
MHDP DRM bridge driver for DisplayPort. Next version of this driver patch
series [3] will use this API.
[3] https://lkml.org/lkml/2020/2/26/263
>
> One of the factors to consider is who will set the attributes; it could
> either be
> phy provider (like 2/2 of this series) or phy consumer (factors like PCIe
> speed,
> lane are usually negotiated with the phy consumer).
>
> Now if phy provider is setting/getting the attributes, then
> phy_set_attrs/phy_get_attrs should be protected by mutex. We don't want
> to be updating attributes when phy consumer is doing some phy ops.
>
> If phy_consumer is updating attributes, it could directly access the phy
> attributes if it's updating within one of those phy ops. Don't really see a
> need
> for using an API to update the attributes then.
>
> However if it's updating outside the phy_ops, then it would still make sense
> to use the APIs to update attributes with all those mutex protection.
Ok. Please check if following implementation is correct in
phy_get_attrs/phy_set_attrs APIs for the above case.
mutex_lock(&phy->mutex);
memcpy(....);
mutex_unlock(&phy->mutex);
Thanks & regards,
Swapnil
>
> Regards
> Kishon
> >
> >> 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 +401,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, struct phy_attrs
> >> +attrs) {
> >> + return;
> >> +}
> >> +
> >> static inline struct phy *phy_get(struct device *dev, const char
> >> *string) {
> >> return ERR_PTR(-ENOSYS);
> >> --
> >> 2.26.1
> >
