| Message ID | 5b67c348-4ec9-58ca-05ed-8b93bed77efb@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi, On Wednesday 20 December 2017 02:11 PM, Manu Gautam wrote: > Hi > > > On 12/20/2017 12:47 PM, Kishon Vijay Abraham I wrote: >> Hi, >> > [snip] >>>> Why not use a notification mechanism instead of adding new APIs in phy-core. >>>> This will only bloat phy-core with APIs for a particular platform. >>> Do you mean notifier_chains ? >>> When we have multiple instances of USB PHYs then notifier chains are not >>> of much help. For any platform glue or PHY driver it will be very difficult to >>> figure out if notification received for speed was for same phy/bus or a >>> different one. >>> Using PHY callbacks looked more elegant to me. Additionally PHY drivers >>> can also use this info decide power management policy e.g. if speed is >>> INVALID then it means PHY is not in a session and it can enter deepest >>> low power state. >>> Additionally if you prefer set_speed name over notify_speed then I am >>> ok with that as well so that it sounds more generic. >> I'd prefer adding modes in enum phy_mode according to speed and using phy_set_mode. > > yeah, that also seems good idea. How about something like this: > > --- a/include/linux/phy/phy.h > +++ b/include/linux/phy/phy.h > @@ -23,12 +23,16 @@ > struct phy; > > enum phy_mode { > - PHY_MODE_INVALID, > - PHY_MODE_USB_HOST, > - PHY_MODE_USB_DEVICE, > - PHY_MODE_USB_OTG, > - PHY_MODE_SGMII, > - PHY_MODE_10GKR, > + PHY_MODE_INVALID = 0, > + PHY_MODE_USB_HOST = BIT(0), > + PHY_MODE_USB_DEVICE = BIT(1), > + PHY_MODE_USB_OTG, = BIT(2), > + PHY_MODE_SGMII = BIT(3), > + PHY_MODE_10GKR = BIT(4), > + PHY_MODE_USB_LS = BIT(5), > + PHY_MODE_USB_FS = BIT(6), > + PHY_MODE_USB_HS = BIT(7), > + PHY_MODE_USB_SS = BIT(8), > }; > > > This way I don't need to duplicate USB speed enums for host/device or otg modes. no.. let's keep enum. It's lot more cleaner IMO. Thanks Kishon -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 12/28/2017 4:34 PM, Kishon Vijay Abraham I wrote: > Hi, > [snip] > >>> I'd prefer adding modes in enum phy_mode according to speed and using phy_set_mode. >> yeah, that also seems good idea. How about something like this: >> >> --- a/include/linux/phy/phy.h >> +++ b/include/linux/phy/phy.h >> @@ -23,12 +23,16 @@ >> struct phy; >> >> enum phy_mode { >> - PHY_MODE_INVALID, >> - PHY_MODE_USB_HOST, >> - PHY_MODE_USB_DEVICE, >> - PHY_MODE_USB_OTG, >> - PHY_MODE_SGMII, >> - PHY_MODE_10GKR, >> + PHY_MODE_INVALID = 0, >> + PHY_MODE_USB_HOST = BIT(0), >> + PHY_MODE_USB_DEVICE = BIT(1), >> + PHY_MODE_USB_OTG, = BIT(2), >> + PHY_MODE_SGMII = BIT(3), >> + PHY_MODE_10GKR = BIT(4), >> + PHY_MODE_USB_LS = BIT(5), >> + PHY_MODE_USB_FS = BIT(6), >> + PHY_MODE_USB_HS = BIT(7), >> + PHY_MODE_USB_SS = BIT(8), >> }; >> >> >> This way I don't need to duplicate USB speed enums for host/device or otg modes. > no.. let's keep enum. It's lot more cleaner IMO. In that case all PHY drivers would need to consider these speed enums. E.g. if (mode == HOST) check in PHY driver would need to be changed to: if (mode == HOST || mode == HOST_LS || mode == HOST_FS || mode == HOST_HS || mode == HOST_SS This looks clumsy. Where as if bits to used then there is no need for such changes. > > Thanks > Kishon > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Friday 29 December 2017 09:54 AM, Manu Gautam wrote: > Hi, > > > On 12/28/2017 4:34 PM, Kishon Vijay Abraham I wrote: >> Hi, >> > [snip] >> >>>> I'd prefer adding modes in enum phy_mode according to speed and using phy_set_mode. >>> yeah, that also seems good idea. How about something like this: >>> >>> --- a/include/linux/phy/phy.h >>> +++ b/include/linux/phy/phy.h >>> @@ -23,12 +23,16 @@ >>> struct phy; >>> >>> enum phy_mode { >>> - PHY_MODE_INVALID, >>> - PHY_MODE_USB_HOST, >>> - PHY_MODE_USB_DEVICE, >>> - PHY_MODE_USB_OTG, >>> - PHY_MODE_SGMII, >>> - PHY_MODE_10GKR, >>> + PHY_MODE_INVALID = 0, >>> + PHY_MODE_USB_HOST = BIT(0), >>> + PHY_MODE_USB_DEVICE = BIT(1), >>> + PHY_MODE_USB_OTG, = BIT(2), >>> + PHY_MODE_SGMII = BIT(3), >>> + PHY_MODE_10GKR = BIT(4), >>> + PHY_MODE_USB_LS = BIT(5), >>> + PHY_MODE_USB_FS = BIT(6), >>> + PHY_MODE_USB_HS = BIT(7), >>> + PHY_MODE_USB_SS = BIT(8), >>> }; >>> >>> >>> This way I don't need to duplicate USB speed enums for host/device or otg modes. >> no.. let's keep enum. It's lot more cleaner IMO. > > In that case all PHY drivers would need to consider these speed enums. E.g. > > if (mode == HOST) check in PHY driver would need to be changed to: > if (mode == HOST || mode == HOST_LS || mode == HOST_FS || mode == HOST_HS || > mode == HOST_SS suggest using switch in such case.. and not all PHY drivers do specific configurations for specific speeds. > > This looks clumsy. > Where as if bits to used then there is no need for such changes. really? using bits should only make it more clumsy. -Kishon -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 12/29/2017 11:58 AM, Kishon Vijay Abraham I wrote: > Hi, > > On Friday 29 December 2017 09:54 AM, Manu Gautam wrote: >> Hi, [snip] > > suggest using switch in such case.. and not all PHY drivers do specific > configurations for specific speeds. >> This looks clumsy. >> Where as if bits to used then there is no need for such changes. > really? using bits should only make it more clumsy. Thanks for your feedback. I will update this in next patchset. > -Kishon
--- a/include/linux/phy/phy.h +++ b/include/linux/phy/phy.h @@ -23,12 +23,16 @@ struct phy; enum phy_mode { - PHY_MODE_INVALID, - PHY_MODE_USB_HOST, - PHY_MODE_USB_DEVICE, - PHY_MODE_USB_OTG, - PHY_MODE_SGMII, - PHY_MODE_10GKR, + PHY_MODE_INVALID = 0, + PHY_MODE_USB_HOST = BIT(0), + PHY_MODE_USB_DEVICE = BIT(1), + PHY_MODE_USB_OTG, = BIT(2), + PHY_MODE_SGMII = BIT(3), + PHY_MODE_10GKR = BIT(4), + PHY_MODE_USB_LS = BIT(5), + PHY_MODE_USB_FS = BIT(6), + PHY_MODE_USB_HS = BIT(7), + PHY_MODE_USB_SS = BIT(8), };