| Message ID | 1386672926-26885-2-git-send-email-gautam.vivek@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Tue, Dec 10, 2013 at 04:25:23PM +0530, Vivek Gautam wrote: > Some PHY controllers may need to tune PHY post-initialization, > so that the PHY consumers can call phy-tuning at appropriate > point of time. > > Signed-off-by: vivek Gautam <gautam.vivek@samsung.com> > --- > drivers/phy/phy-core.c | 20 ++++++++++++++++++++ > include/linux/phy/phy.h | 7 +++++++ > 2 files changed, 27 insertions(+), 0 deletions(-) > > diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c > index 03cf8fb..68dbb90 100644 > --- a/drivers/phy/phy-core.c > +++ b/drivers/phy/phy-core.c > @@ -239,6 +239,26 @@ out: > } > EXPORT_SYMBOL_GPL(phy_power_off); > > +int phy_tune(struct phy *phy) > +{ > + int ret = -ENOTSUPP; > + > + mutex_lock(&phy->mutex); > + if (phy->ops->tune) { > + ret = phy->ops->tune(phy); > + if (ret < 0) { > + dev_err(&phy->dev, "phy tuning failed --> %d\n", ret); > + goto out; > + } > + } > + > +out: > + mutex_unlock(&phy->mutex); > + > + return ret; > +} > +EXPORT_SYMBOL_GPL(phy_tune); I think "setup" instead of "tune" is much more clear and reusable. Thanks,
Hi, On Tue, Dec 10, 2013 at 7:31 PM, Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > Hi, Thanks for reviewing this. > > On Tue, Dec 10, 2013 at 04:25:23PM +0530, Vivek Gautam wrote: >> Some PHY controllers may need to tune PHY post-initialization, >> so that the PHY consumers can call phy-tuning at appropriate >> point of time. >> >> Signed-off-by: vivek Gautam <gautam.vivek@samsung.com> >> --- >> drivers/phy/phy-core.c | 20 ++++++++++++++++++++ >> include/linux/phy/phy.h | 7 +++++++ >> 2 files changed, 27 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c >> index 03cf8fb..68dbb90 100644 >> --- a/drivers/phy/phy-core.c >> +++ b/drivers/phy/phy-core.c >> @@ -239,6 +239,26 @@ out: >> } >> EXPORT_SYMBOL_GPL(phy_power_off); >> >> +int phy_tune(struct phy *phy) >> +{ >> + int ret = -ENOTSUPP; >> + >> + mutex_lock(&phy->mutex); >> + if (phy->ops->tune) { >> + ret = phy->ops->tune(phy); >> + if (ret < 0) { >> + dev_err(&phy->dev, "phy tuning failed --> %d\n", ret); >> + goto out; >> + } >> + } >> + >> +out: >> + mutex_unlock(&phy->mutex); >> + >> + return ret; >> +} >> +EXPORT_SYMBOL_GPL(phy_tune); > > I think "setup" instead of "tune" is much more clear and reusable. I think "setup" will look more like first time setting up the phy, which is rather served by "init" callback. This i thought would serve the purpose of over-riding certain PHY parameters, which would not have been possible at "init" time. Please correct my thinking if i am unable to understand your point here. > > Thanks, > > -- > heikki > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Wed, Dec 11, 2013 at 12:08:04PM +0530, Vivek Gautam wrote: > On Tue, Dec 10, 2013 at 7:31 PM, Heikki Krogerus > > I think "setup" instead of "tune" is much more clear and reusable. > > I think "setup" will look more like first time setting up the phy, > which is rather served by "init" callback. > This i thought would serve the purpose of over-riding certain PHY > parameters, which would not have been > possible at "init" time. > Please correct my thinking if i am unable to understand your point here. OK, sorry I was not clear on this. I'm thinking the same, that this is something that is called later, for example when the controller is ready. Some ULPI phys need to be initialized, but since the controller provides the interface, it's usually not possible during init time. This hook could be used in that case as well. All I'm saying is that "tune" is a poor expression. You will need to add a comment explaining what the hook does in any case, so you'll have something like "this is something that is called when the controller is ready" or something similar. That will make it clear what it's meant for. Thanks,
On Wednesday 11 December 2013 12:08 PM, Vivek Gautam wrote: > Hi, > > > On Tue, Dec 10, 2013 at 7:31 PM, Heikki Krogerus > <heikki.krogerus@linux.intel.com> wrote: >> Hi, > > Thanks for reviewing this. > >> >> On Tue, Dec 10, 2013 at 04:25:23PM +0530, Vivek Gautam wrote: >>> Some PHY controllers may need to tune PHY post-initialization, >>> so that the PHY consumers can call phy-tuning at appropriate >>> point of time. >>> >>> Signed-off-by: vivek Gautam <gautam.vivek@samsung.com> >>> --- >>> drivers/phy/phy-core.c | 20 ++++++++++++++++++++ >>> include/linux/phy/phy.h | 7 +++++++ >>> 2 files changed, 27 insertions(+), 0 deletions(-) >>> >>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c >>> index 03cf8fb..68dbb90 100644 >>> --- a/drivers/phy/phy-core.c >>> +++ b/drivers/phy/phy-core.c >>> @@ -239,6 +239,26 @@ out: >>> } >>> EXPORT_SYMBOL_GPL(phy_power_off); >>> >>> +int phy_tune(struct phy *phy) >>> +{ >>> + int ret = -ENOTSUPP; >>> + >>> + mutex_lock(&phy->mutex); >>> + if (phy->ops->tune) { >>> + ret = phy->ops->tune(phy); >>> + if (ret < 0) { >>> + dev_err(&phy->dev, "phy tuning failed --> %d\n", ret); >>> + goto out; >>> + } >>> + } >>> + >>> +out: >>> + mutex_unlock(&phy->mutex); >>> + >>> + return ret; >>> +} >>> +EXPORT_SYMBOL_GPL(phy_tune); >> >> I think "setup" instead of "tune" is much more clear and reusable. > > I think "setup" will look more like first time setting up the phy, > which is rather served by "init" callback. > This i thought would serve the purpose of over-riding certain PHY > parameters, which would not have been > possible at "init" time. > Please correct my thinking if i am unable to understand your point here. how about 'calibrate'? Thanks Kishon > >> >> Thanks, >> >> -- >> heikki >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > > -- To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On Wed, Dec 11, 2013 at 1:39 PM, Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > Hi, > > On Wed, Dec 11, 2013 at 12:08:04PM +0530, Vivek Gautam wrote: >> On Tue, Dec 10, 2013 at 7:31 PM, Heikki Krogerus >> > I think "setup" instead of "tune" is much more clear and reusable. >> >> I think "setup" will look more like first time setting up the phy, >> which is rather served by "init" callback. >> This i thought would serve the purpose of over-riding certain PHY >> parameters, which would not have been >> possible at "init" time. >> Please correct my thinking if i am unable to understand your point here. > > OK, sorry I was not clear on this. I'm thinking the same, that this is > something that is called later, for example when the controller is > ready. Some ULPI phys need to be initialized, but since the controller > provides the interface, it's usually not possible during init time. > This hook could be used in that case as well. > > All I'm saying is that "tune" is a poor expression. You will need to > add a comment explaining what the hook does in any case, so you'll > have something like "this is something that is called when the > controller is ready" or something similar. That will make it clear > what it's meant for. Ok, i think i should have kept a comment atleast :-( I will add proper comments above, and as suggested in the mail by Kishon, may be name it calibrate ? What do you think ? > > Thanks, > > -- > heikki
Hi Kishon, On Wed, Dec 11, 2013 at 1:47 PM, Kishon Vijay Abraham I <kishon@ti.com> wrote: > On Wednesday 11 December 2013 12:08 PM, Vivek Gautam wrote: >> Hi, >> >> >> On Tue, Dec 10, 2013 at 7:31 PM, Heikki Krogerus >> <heikki.krogerus@linux.intel.com> wrote: >>> Hi, >> >> Thanks for reviewing this. >> >>> >>> On Tue, Dec 10, 2013 at 04:25:23PM +0530, Vivek Gautam wrote: >>>> Some PHY controllers may need to tune PHY post-initialization, >>>> so that the PHY consumers can call phy-tuning at appropriate >>>> point of time. >>>> >>>> Signed-off-by: vivek Gautam <gautam.vivek@samsung.com> >>>> --- >>>> drivers/phy/phy-core.c | 20 ++++++++++++++++++++ >>>> include/linux/phy/phy.h | 7 +++++++ >>>> 2 files changed, 27 insertions(+), 0 deletions(-) >>>> >>>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c >>>> index 03cf8fb..68dbb90 100644 >>>> --- a/drivers/phy/phy-core.c >>>> +++ b/drivers/phy/phy-core.c >>>> @@ -239,6 +239,26 @@ out: >>>> } >>>> EXPORT_SYMBOL_GPL(phy_power_off); >>>> >>>> +int phy_tune(struct phy *phy) >>>> +{ >>>> + int ret = -ENOTSUPP; >>>> + >>>> + mutex_lock(&phy->mutex); >>>> + if (phy->ops->tune) { >>>> + ret = phy->ops->tune(phy); >>>> + if (ret < 0) { >>>> + dev_err(&phy->dev, "phy tuning failed --> %d\n", ret); >>>> + goto out; >>>> + } >>>> + } >>>> + >>>> +out: >>>> + mutex_unlock(&phy->mutex); >>>> + >>>> + return ret; >>>> +} >>>> +EXPORT_SYMBOL_GPL(phy_tune); >>> >>> I think "setup" instead of "tune" is much more clear and reusable. >> >> I think "setup" will look more like first time setting up the phy, >> which is rather served by "init" callback. >> This i thought would serve the purpose of over-riding certain PHY >> parameters, which would not have been >> possible at "init" time. >> Please correct my thinking if i am unable to understand your point here. > > how about 'calibrate'? Hmm, seems like a better name :-) > > Thanks > Kishon >> >>> >>> Thanks, >>> >>> -- >>> heikki >>> -- >>> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> >> >
Hi again, On Wed, Dec 11, 2013 at 02:02:43PM +0530, Vivek Gautam wrote: > On Wed, Dec 11, 2013 at 1:39 PM, Heikki Krogerus > <heikki.krogerus@linux.intel.com> wrote: > > On Wed, Dec 11, 2013 at 12:08:04PM +0530, Vivek Gautam wrote: > >> On Tue, Dec 10, 2013 at 7:31 PM, Heikki Krogerus > >> > I think "setup" instead of "tune" is much more clear and reusable. > >> > >> I think "setup" will look more like first time setting up the phy, > >> which is rather served by "init" callback. > >> This i thought would serve the purpose of over-riding certain PHY > >> parameters, which would not have been > >> possible at "init" time. > >> Please correct my thinking if i am unable to understand your point here. > > > > OK, sorry I was not clear on this. I'm thinking the same, that this is > > something that is called later, for example when the controller is > > ready. Some ULPI phys need to be initialized, but since the controller > > provides the interface, it's usually not possible during init time. > > This hook could be used in that case as well. > > > > All I'm saying is that "tune" is a poor expression. You will need to > > add a comment explaining what the hook does in any case, so you'll > > have something like "this is something that is called when the > > controller is ready" or something similar. That will make it clear > > what it's meant for. > > Ok, i think i should have kept a comment atleast :-( > I will add proper comments above, and as suggested in the mail by > Kishon, may be name it calibrate ? > What do you think ? Sure, I'm fine with that. Thanks,
diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c index 03cf8fb..68dbb90 100644 --- a/drivers/phy/phy-core.c +++ b/drivers/phy/phy-core.c @@ -239,6 +239,26 @@ out: } EXPORT_SYMBOL_GPL(phy_power_off); +int phy_tune(struct phy *phy) +{ + int ret = -ENOTSUPP; + + mutex_lock(&phy->mutex); + if (phy->ops->tune) { + ret = phy->ops->tune(phy); + if (ret < 0) { + dev_err(&phy->dev, "phy tuning failed --> %d\n", ret); + goto out; + } + } + +out: + mutex_unlock(&phy->mutex); + + return ret; +} +EXPORT_SYMBOL_GPL(phy_tune); + /** * of_phy_get() - lookup and obtain a reference to a phy by phandle * @dev: device that requests this phy diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h index 6d72269..d3b32b0 100644 --- a/include/linux/phy/phy.h +++ b/include/linux/phy/phy.h @@ -34,6 +34,7 @@ struct phy_ops { int (*exit)(struct phy *phy); int (*power_on)(struct phy *phy); int (*power_off)(struct phy *phy); + int (*tune)(struct phy *phy); struct module *owner; }; @@ -127,6 +128,7 @@ int phy_init(struct phy *phy); int phy_exit(struct phy *phy); int phy_power_on(struct phy *phy); int phy_power_off(struct phy *phy); +int phy_tune(struct phy *phy); struct phy *phy_get(struct device *dev, const char *string); struct phy *devm_phy_get(struct device *dev, const char *string); void phy_put(struct phy *phy); @@ -199,6 +201,11 @@ static inline int phy_power_off(struct phy *phy) return -ENOSYS; } +static inline int phy_tune(struct phy *phy) +{ + return -ENOSYS; +} + static inline struct phy *phy_get(struct device *dev, const char *string) { return ERR_PTR(-ENOSYS);
Some PHY controllers may need to tune PHY post-initialization, so that the PHY consumers can call phy-tuning at appropriate point of time. Signed-off-by: vivek Gautam <gautam.vivek@samsung.com> --- drivers/phy/phy-core.c | 20 ++++++++++++++++++++ include/linux/phy/phy.h | 7 +++++++ 2 files changed, 27 insertions(+), 0 deletions(-)