This patch set adds a new API to get phy by index when multiple
phys are present. This patch is based on discussion with Arnd Bergmann
about dt bindings for multiple phys.
History:
v1:
- Removed null pointers on Dmitry's suggestion
- Improved documentation in commit messages
- Exported new phy api
v2:
- EHCI and OHCI platform Kconfigs select Generic Phy
to fix build errors in certain configs.
Arun Ramamurthy (3):
phy: core: Add devm_of_phy_get_by_index to phy-core
usb: ehci-platform: Use devm_of_phy_get_by_index
usb: ohci-platform: Use devm_of_phy_get_by_index
drivers/phy/phy-core.c | 32 ++++++++++++++++++
drivers/usb/host/Kconfig | 2 ++
drivers/usb/host/ehci-platform.c | 70 ++++++++++++++--------------------------
drivers/usb/host/ohci-platform.c | 70 ++++++++++++++--------------------------
include/linux/phy/phy.h | 2 ++
5 files changed, 86 insertions(+), 90 deletions(-)
--
2.3.4
Some generic drivers, such as ehci, may use multiple phys and for such
drivers referencing phy(s) by name(s) does not make sense. Instead of
inventing new naming schemes and using custom code to iterate through them,
such drivers are better of using nameless phy bindings and using this newly
introduced API to iterate through them.
Signed-off-by: Arun Ramamurthy <[email protected]>
Reviewed-by: Ray Jui <[email protected]>
Reviewed-by: Scott Branden <[email protected]>
---
drivers/phy/phy-core.c | 32 ++++++++++++++++++++++++++++++++
include/linux/phy/phy.h | 2 ++
2 files changed, 34 insertions(+)
diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index 3791838..964a84d 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -623,6 +623,38 @@ struct phy *devm_of_phy_get(struct device *dev, struct device_node *np,
EXPORT_SYMBOL_GPL(devm_of_phy_get);
/**
+ * devm_of_phy_get_by_index() - lookup and obtain a reference to a phy by index.
+ * @dev: device that requests this phy
+ * @np: node containing the phy
+ * @index: index of the phy
+ *
+ * Gets the phy using _of_phy_get(), and associates a device with it using
+ * devres. On driver detach, release function is invoked on the devres data,
+ * then, devres data is freed.
+ *
+ */
+struct phy *devm_of_phy_get_by_index(struct device *dev, struct device_node *np,
+ int index)
+{
+ struct phy **ptr, *phy;
+
+ ptr = devres_alloc(devm_phy_release, sizeof(*ptr), GFP_KERNEL);
+ if (!ptr)
+ return ERR_PTR(-ENOMEM);
+
+ phy = _of_phy_get(np, index);
+ if (!IS_ERR(phy)) {
+ *ptr = phy;
+ devres_add(dev, ptr);
+ } else {
+ devres_free(ptr);
+ }
+
+ return phy;
+}
+EXPORT_SYMBOL_GPL(devm_of_phy_get_by_index);
+
+/**
* phy_create() - create a new phy
* @dev: device that is creating the new phy
* @node: device node of the phy
diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
index a0197fa..ae2ffaf 100644
--- a/include/linux/phy/phy.h
+++ b/include/linux/phy/phy.h
@@ -133,6 +133,8 @@ struct phy *devm_phy_get(struct device *dev, const char *string);
struct phy *devm_phy_optional_get(struct device *dev, const char *string);
struct phy *devm_of_phy_get(struct device *dev, struct device_node *np,
const char *con_id);
+struct phy *devm_of_phy_get_by_index(struct device *dev, struct device_node *np,
+ int index);
void phy_put(struct phy *phy);
void devm_phy_put(struct device *dev, struct phy *phy);
struct phy *of_phy_get(struct device_node *np, const char *con_id);
--
2.3.4
Getting phys by index instead of phy names so that we do
not have to create a naming scheme when multiple phys
are present
Signed-off-by: Arun Ramamurthy <[email protected]>
Reviewed-by: Ray Jui <[email protected]>
Reviewed-by: Scott Branden <[email protected]>
---
drivers/usb/host/Kconfig | 1 +
drivers/usb/host/ehci-platform.c | 70 ++++++++++++++--------------------------
2 files changed, 26 insertions(+), 45 deletions(-)
diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index 5ad60e4..563f22d 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -284,6 +284,7 @@ config USB_EHCI_ATH79
config USB_EHCI_HCD_PLATFORM
tristate "Generic EHCI driver for a platform device"
+ select GENERIC_PHY
default n
---help---
Adds an EHCI host driver for a generic platform device, which
diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
index d8a75a5..a7563b9 100644
--- a/drivers/usb/host/ehci-platform.c
+++ b/drivers/usb/host/ehci-platform.c
@@ -88,15 +88,13 @@ static int ehci_platform_power_on(struct platform_device *dev)
}
for (phy_num = 0; phy_num < priv->num_phys; phy_num++) {
- if (priv->phys[phy_num]) {
- ret = phy_init(priv->phys[phy_num]);
- if (ret)
- goto err_exit_phy;
- ret = phy_power_on(priv->phys[phy_num]);
- if (ret) {
- phy_exit(priv->phys[phy_num]);
- goto err_exit_phy;
- }
+ ret = phy_init(priv->phys[phy_num]);
+ if (ret)
+ goto err_exit_phy;
+ ret = phy_power_on(priv->phys[phy_num]);
+ if (ret) {
+ phy_exit(priv->phys[phy_num]);
+ goto err_exit_phy;
}
}
@@ -104,10 +102,8 @@ static int ehci_platform_power_on(struct platform_device *dev)
err_exit_phy:
while (--phy_num >= 0) {
- if (priv->phys[phy_num]) {
- phy_power_off(priv->phys[phy_num]);
- phy_exit(priv->phys[phy_num]);
- }
+ phy_power_off(priv->phys[phy_num]);
+ phy_exit(priv->phys[phy_num]);
}
err_disable_clks:
while (--clk >= 0)
@@ -123,10 +119,8 @@ static void ehci_platform_power_off(struct platform_device *dev)
int clk, phy_num;
for (phy_num = 0; phy_num < priv->num_phys; phy_num++) {
- if (priv->phys[phy_num]) {
- phy_power_off(priv->phys[phy_num]);
- phy_exit(priv->phys[phy_num]);
- }
+ phy_power_off(priv->phys[phy_num]);
+ phy_exit(priv->phys[phy_num]);
}
for (clk = EHCI_MAX_CLKS - 1; clk >= 0; clk--)
@@ -154,7 +148,6 @@ static int ehci_platform_probe(struct platform_device *dev)
struct usb_ehci_pdata *pdata = dev_get_platdata(&dev->dev);
struct ehci_platform_priv *priv;
struct ehci_hcd *ehci;
- const char *phy_name;
int err, irq, phy_num, clk = 0;
if (usb_disabled())
@@ -204,36 +197,23 @@ static int ehci_platform_probe(struct platform_device *dev)
priv->num_phys = of_count_phandle_with_args(dev->dev.of_node,
"phys", "#phy-cells");
- priv->num_phys = priv->num_phys > 0 ? priv->num_phys : 1;
- priv->phys = devm_kcalloc(&dev->dev, priv->num_phys,
- sizeof(struct phy *), GFP_KERNEL);
- if (!priv->phys)
- return -ENOMEM;
+ if (priv->num_phys > 0) {
+ priv->phys = devm_kcalloc(&dev->dev, priv->num_phys,
+ sizeof(struct phy *), GFP_KERNEL);
+ if (!priv->phys)
+ return -ENOMEM;
+ } else
+ priv->num_phys = 0;
for (phy_num = 0; phy_num < priv->num_phys; phy_num++) {
- err = of_property_read_string_index(
- dev->dev.of_node,
- "phy-names", phy_num,
- &phy_name);
-
- if (err < 0) {
- if (priv->num_phys > 1) {
- dev_err(&dev->dev, "phy-names not provided");
- goto err_put_hcd;
- } else
- phy_name = "usb";
- }
-
- priv->phys[phy_num] = devm_phy_get(&dev->dev,
- phy_name);
- if (IS_ERR(priv->phys[phy_num])) {
- err = PTR_ERR(priv->phys[phy_num]);
- if ((priv->num_phys > 1) ||
- (err == -EPROBE_DEFER))
- goto err_put_hcd;
- priv->phys[phy_num] = NULL;
- }
+ priv->phys[phy_num] = devm_of_phy_get_by_index(&dev->dev
+ , dev->dev.of_node
+ , phy_num);
+ if (IS_ERR(priv->phys[phy_num])) {
+ err = PTR_ERR(priv->phys[phy_num]);
+ goto err_put_hcd;
+ }
}
for (clk = 0; clk < EHCI_MAX_CLKS; clk++) {
--
2.3.4
Getting phys by index instead of phy names so that we do
not have to create a naming scheme when multiple phys are present
Signed-off-by: Arun Ramamurthy <[email protected]>
Reviewed-by: Ray Jui <[email protected]>
Reviewed-by: Scott Branden <[email protected]>
---
drivers/usb/host/Kconfig | 1 +
drivers/usb/host/ohci-platform.c | 70 ++++++++++++++--------------------------
2 files changed, 26 insertions(+), 45 deletions(-)
diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index 563f22d..1a8869c 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -558,6 +558,7 @@ config USB_CNS3XXX_OHCI
config USB_OHCI_HCD_PLATFORM
tristate "Generic OHCI driver for a platform device"
+ select GENERIC_PHY
default n
---help---
Adds an OHCI host driver for a generic platform device, which
diff --git a/drivers/usb/host/ohci-platform.c b/drivers/usb/host/ohci-platform.c
index 185ceee..0f8edf6 100644
--- a/drivers/usb/host/ohci-platform.c
+++ b/drivers/usb/host/ohci-platform.c
@@ -57,15 +57,13 @@ static int ohci_platform_power_on(struct platform_device *dev)
}
for (phy_num = 0; phy_num < priv->num_phys; phy_num++) {
- if (priv->phys[phy_num]) {
- ret = phy_init(priv->phys[phy_num]);
- if (ret)
- goto err_exit_phy;
- ret = phy_power_on(priv->phys[phy_num]);
- if (ret) {
- phy_exit(priv->phys[phy_num]);
- goto err_exit_phy;
- }
+ ret = phy_init(priv->phys[phy_num]);
+ if (ret)
+ goto err_exit_phy;
+ ret = phy_power_on(priv->phys[phy_num]);
+ if (ret) {
+ phy_exit(priv->phys[phy_num]);
+ goto err_exit_phy;
}
}
@@ -73,10 +71,8 @@ static int ohci_platform_power_on(struct platform_device *dev)
err_exit_phy:
while (--phy_num >= 0) {
- if (priv->phys[phy_num]) {
- phy_power_off(priv->phys[phy_num]);
- phy_exit(priv->phys[phy_num]);
- }
+ phy_power_off(priv->phys[phy_num]);
+ phy_exit(priv->phys[phy_num]);
}
err_disable_clks:
while (--clk >= 0)
@@ -92,10 +88,8 @@ static void ohci_platform_power_off(struct platform_device *dev)
int clk, phy_num;
for (phy_num = 0; phy_num < priv->num_phys; phy_num++) {
- if (priv->phys[phy_num]) {
- phy_power_off(priv->phys[phy_num]);
- phy_exit(priv->phys[phy_num]);
- }
+ phy_power_off(priv->phys[phy_num]);
+ phy_exit(priv->phys[phy_num]);
}
for (clk = OHCI_MAX_CLKS - 1; clk >= 0; clk--)
@@ -123,7 +117,6 @@ static int ohci_platform_probe(struct platform_device *dev)
struct usb_ohci_pdata *pdata = dev_get_platdata(&dev->dev);
struct ohci_platform_priv *priv;
struct ohci_hcd *ohci;
- const char *phy_name;
int err, irq, phy_num, clk = 0;
if (usb_disabled())
@@ -174,36 +167,23 @@ static int ohci_platform_probe(struct platform_device *dev)
priv->num_phys = of_count_phandle_with_args(dev->dev.of_node,
"phys", "#phy-cells");
- priv->num_phys = priv->num_phys > 0 ? priv->num_phys : 1;
- priv->phys = devm_kcalloc(&dev->dev, priv->num_phys,
- sizeof(struct phy *), GFP_KERNEL);
- if (!priv->phys)
- return -ENOMEM;
+ if (priv->num_phys > 0) {
+ priv->phys = devm_kcalloc(&dev->dev, priv->num_phys,
+ sizeof(struct phy *), GFP_KERNEL);
+ if (!priv->phys)
+ return -ENOMEM;
+ } else
+ priv->num_phys = 0;
for (phy_num = 0; phy_num < priv->num_phys; phy_num++) {
- err = of_property_read_string_index(
- dev->dev.of_node,
- "phy-names", phy_num,
- &phy_name);
-
- if (err < 0) {
- if (priv->num_phys > 1) {
- dev_err(&dev->dev, "phy-names not provided");
- goto err_put_hcd;
- } else
- phy_name = "usb";
- }
-
- priv->phys[phy_num] = devm_phy_get(&dev->dev,
- phy_name);
- if (IS_ERR(priv->phys[phy_num])) {
- err = PTR_ERR(priv->phys[phy_num]);
- if ((priv->num_phys > 1) ||
- (err == -EPROBE_DEFER))
- goto err_put_hcd;
- priv->phys[phy_num] = NULL;
- }
+ priv->phys[phy_num] = devm_of_phy_get_by_index(&dev->dev
+ , dev->dev.of_node
+ , phy_num);
+ if (IS_ERR(priv->phys[phy_num])) {
+ err = PTR_ERR(priv->phys[phy_num]);
+ goto err_put_hcd;
+ }
}
for (clk = 0; clk < OHCI_MAX_CLKS; clk++) {
--
2.3.4
On Mon, Apr 13, 2015 at 03:10:46PM -0700, Arun Ramamurthy wrote:
> Getting phys by index instead of phy names so that we do
> not have to create a naming scheme when multiple phys
> are present
>
> Signed-off-by: Arun Ramamurthy <[email protected]>
> Reviewed-by: Ray Jui <[email protected]>
> Reviewed-by: Scott Branden <[email protected]>
> ---
> drivers/usb/host/Kconfig | 1 +
> drivers/usb/host/ehci-platform.c | 70 ++++++++++++++--------------------------
> 2 files changed, 26 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> index 5ad60e4..563f22d 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -284,6 +284,7 @@ config USB_EHCI_ATH79
>
> config USB_EHCI_HCD_PLATFORM
> tristate "Generic EHCI driver for a platform device"
> + select GENERIC_PHY
Configs should never select if at all possible.
On Tuesday 14 April 2015 13:19:34 Greg Kroah-Hartman wrote:
> On Mon, Apr 13, 2015 at 03:10:46PM -0700, Arun Ramamurthy wrote:
> > Getting phys by index instead of phy names so that we do
> > not have to create a naming scheme when multiple phys
> > are present
> >
> > Signed-off-by: Arun Ramamurthy <[email protected]>
> > Reviewed-by: Ray Jui <[email protected]>
> > Reviewed-by: Scott Branden <[email protected]>
> > ---
> > drivers/usb/host/Kconfig | 1 +
> > drivers/usb/host/ehci-platform.c | 70 ++++++++++++++--------------------------
> > 2 files changed, 26 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> > index 5ad60e4..563f22d 100644
> > --- a/drivers/usb/host/Kconfig
> > +++ b/drivers/usb/host/Kconfig
> > @@ -284,6 +284,7 @@ config USB_EHCI_ATH79
> >
> > config USB_EHCI_HCD_PLATFORM
> > tristate "Generic EHCI driver for a platform device"
> > + select GENERIC_PHY
>
> Configs should never select if at all possible.
>
This is true, but all other drivers do the same for GENERIC_PHY at the
moment. If this one gets changed, we should probably apply the same
solution to all current users and fix them consistently.
We can do one of these two:
a) make sure that the framework has 'static inline' stubs that let you
build all drivers using it when the framework itself is disabled.
b) change the drivers using it to 'depends on', and make GENERIC_PHY
itself a hidden option without a Kconfig prompt.
Either solution is probably simple enough that it can be done as
part of this series.
Arnd
On Tue, Apr 14, 2015 at 01:33:08PM +0200, Arnd Bergmann wrote:
> On Tuesday 14 April 2015 13:19:34 Greg Kroah-Hartman wrote:
> > On Mon, Apr 13, 2015 at 03:10:46PM -0700, Arun Ramamurthy wrote:
> > > Getting phys by index instead of phy names so that we do
> > > not have to create a naming scheme when multiple phys
> > > are present
> > >
> > > Signed-off-by: Arun Ramamurthy <[email protected]>
> > > Reviewed-by: Ray Jui <[email protected]>
> > > Reviewed-by: Scott Branden <[email protected]>
> > > ---
> > > drivers/usb/host/Kconfig | 1 +
> > > drivers/usb/host/ehci-platform.c | 70 ++++++++++++++--------------------------
> > > 2 files changed, 26 insertions(+), 45 deletions(-)
> > >
> > > diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> > > index 5ad60e4..563f22d 100644
> > > --- a/drivers/usb/host/Kconfig
> > > +++ b/drivers/usb/host/Kconfig
> > > @@ -284,6 +284,7 @@ config USB_EHCI_ATH79
> > >
> > > config USB_EHCI_HCD_PLATFORM
> > > tristate "Generic EHCI driver for a platform device"
> > > + select GENERIC_PHY
> >
> > Configs should never select if at all possible.
> >
>
> This is true, but all other drivers do the same for GENERIC_PHY at the
> moment. If this one gets changed, we should probably apply the same
> solution to all current users and fix them consistently.
>
> We can do one of these two:
>
> a) make sure that the framework has 'static inline' stubs that let you
> build all drivers using it when the framework itself is disabled.
Yes, please do that.
> b) change the drivers using it to 'depends on', and make GENERIC_PHY
> itself a hidden option without a Kconfig prompt.
Then how could GENERIC_PHY ever get set?
> Either solution is probably simple enough that it can be done as
> part of this series.
That's fine with me, but please no more selects.
thanks,
greg k-h
On Tuesday 14 April 2015 14:37:37 Greg Kroah-Hartman wrote:
> On Tue, Apr 14, 2015 at 01:33:08PM +0200, Arnd Bergmann wrote:
> > This is true, but all other drivers do the same for GENERIC_PHY at the
> > moment. If this one gets changed, we should probably apply the same
> > solution to all current users and fix them consistently.
> >
> > We can do one of these two:
> >
> > a) make sure that the framework has 'static inline' stubs that let you
> > build all drivers using it when the framework itself is disabled.
>
> Yes, please do that.
>
> > b) change the drivers using it to 'depends on', and make GENERIC_PHY
> > itself a hidden option without a Kconfig prompt.
>
> Then how could GENERIC_PHY ever get set?
Right now, every driver that provides a phy uses 'select GENERIC_PHY',
and they would have to keep doing that. This is not unlike what we
do for other silent symbols like MFD_CORE, REGMAP_I2C, or PINCTRL,
and it's not as problematic as 'select' on a user-visible option,
or (worst) mixing 'select' and 'depends on'.
Arnd
On Tue, Apr 14, 2015 at 03:17:30PM +0200, Arnd Bergmann wrote:
> On Tuesday 14 April 2015 14:37:37 Greg Kroah-Hartman wrote:
> > On Tue, Apr 14, 2015 at 01:33:08PM +0200, Arnd Bergmann wrote:
> > > This is true, but all other drivers do the same for GENERIC_PHY at the
> > > moment. If this one gets changed, we should probably apply the same
> > > solution to all current users and fix them consistently.
> > >
> > > We can do one of these two:
> > >
> > > a) make sure that the framework has 'static inline' stubs that let you
> > > build all drivers using it when the framework itself is disabled.
> >
> > Yes, please do that.
> >
> > > b) change the drivers using it to 'depends on', and make GENERIC_PHY
> > > itself a hidden option without a Kconfig prompt.
> >
> > Then how could GENERIC_PHY ever get set?
>
> Right now, every driver that provides a phy uses 'select GENERIC_PHY',
> and they would have to keep doing that. This is not unlike what we
> do for other silent symbols like MFD_CORE, REGMAP_I2C, or PINCTRL,
> and it's not as problematic as 'select' on a user-visible option,
> or (worst) mixing 'select' and 'depends on'.
Ok, that would make more sense, but it would be good for the PHY
maintainer to agree to it as well :)
thanks,
greg k-h
Hi,
On Tuesday 14 April 2015 06:57 PM, Greg Kroah-Hartman wrote:
> On Tue, Apr 14, 2015 at 03:17:30PM +0200, Arnd Bergmann wrote:
>> On Tuesday 14 April 2015 14:37:37 Greg Kroah-Hartman wrote:
>>> On Tue, Apr 14, 2015 at 01:33:08PM +0200, Arnd Bergmann wrote:
>>>> This is true, but all other drivers do the same for GENERIC_PHY at the
>>>> moment. If this one gets changed, we should probably apply the same
>>>> solution to all current users and fix them consistently.
>>>>
>>>> We can do one of these two:
>>>>
>>>> a) make sure that the framework has 'static inline' stubs that let you
>>>> build all drivers using it when the framework itself is disabled.
>>>
>>> Yes, please do that.
>>>
>>>> b) change the drivers using it to 'depends on', and make GENERIC_PHY
>>>> itself a hidden option without a Kconfig prompt.
>>>
>>> Then how could GENERIC_PHY ever get set?
>>
>> Right now, every driver that provides a phy uses 'select GENERIC_PHY',
>> and they would have to keep doing that. This is not unlike what we
>> do for other silent symbols like MFD_CORE, REGMAP_I2C, or PINCTRL,
>> and it's not as problematic as 'select' on a user-visible option,
>> or (worst) mixing 'select' and 'depends on'.
>
> Ok, that would make more sense, but it would be good for the PHY
> maintainer to agree to it as well :)
looking at [1], we should use select only for non-visible symbols and for
symbols with no dependencies. As such GENERIC_PHY is not dependent on other
symbols but for now it is "visible".
phy-core has all the stubs already implemented in include/linux/phy/phy.h. So
removing select GENERIC_PHY shouldn't be a problem. But then it might break a
few platforms where GENERIC_PHY is indirectly enabled by selecting the config
of the driver (using default defconfigs in arch/arm/configs).
The simplest thing would be to make GENERIC_PHY an invisible option?
[1] ->
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/kbuild/kconfig-language.txt#n111
Thanks
Kishon
Hi Arnd,
On Tuesday 14 April 2015 06:47 PM, Arnd Bergmann wrote:
> On Tuesday 14 April 2015 14:37:37 Greg Kroah-Hartman wrote:
>> On Tue, Apr 14, 2015 at 01:33:08PM +0200, Arnd Bergmann wrote:
>>> This is true, but all other drivers do the same for GENERIC_PHY at the
>>> moment. If this one gets changed, we should probably apply the same
>>> solution to all current users and fix them consistently.
>>>
>>> We can do one of these two:
>>>
>>> a) make sure that the framework has 'static inline' stubs that let you
>>> build all drivers using it when the framework itself is disabled.
>>
>> Yes, please do that.
>>
>>> b) change the drivers using it to 'depends on', and make GENERIC_PHY
>>> itself a hidden option without a Kconfig prompt.
>>
>> Then how could GENERIC_PHY ever get set?
>
> Right now, every driver that provides a phy uses 'select GENERIC_PHY',
> and they would have to keep doing that. This is not unlike what we
> do for other silent symbols like MFD_CORE, REGMAP_I2C, or PINCTRL,
> and it's not as problematic as 'select' on a user-visible option,
> or (worst) mixing 'select' and 'depends on'.
Sorry, didn't get how GENERIC_PHY could be set with option 'b'.
Thanks
Kishon
On Mon, 13 Apr 2015, Arun Ramamurthy wrote:
> Getting phys by index instead of phy names so that we do
> not have to create a naming scheme when multiple phys
> are present
>
> Signed-off-by: Arun Ramamurthy <[email protected]>
> Reviewed-by: Ray Jui <[email protected]>
> Reviewed-by: Scott Branden <[email protected]>
You have not responded to the comments I posted earlier:
http://marc.info/?l=linux-usb&m=142798455925594&w=2
Alan Stern
My apologies Alan, I missed that comment I was indeed trying to avoid
the 80 column rule. It looks like i will have to resend this patch, so i
will reformat the code then. Thanks
On 15-04-14 07:41 AM, Alan Stern wrote:
> On Mon, 13 Apr 2015, Arun Ramamurthy wrote:
>
>> Getting phys by index instead of phy names so that we do
>> not have to create a naming scheme when multiple phys
>> are present
>>
>> Signed-off-by: Arun Ramamurthy <[email protected]>
>> Reviewed-by: Ray Jui <[email protected]>
>> Reviewed-by: Scott Branden <[email protected]>
>
> You have not responded to the comments I posted earlier:
>
> http://marc.info/?l=linux-usb&m=142798455925594&w=2
>
> Alan Stern
>
On 15-04-14 07:21 AM, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Tuesday 14 April 2015 06:57 PM, Greg Kroah-Hartman wrote:
>> On Tue, Apr 14, 2015 at 03:17:30PM +0200, Arnd Bergmann wrote:
>>> On Tuesday 14 April 2015 14:37:37 Greg Kroah-Hartman wrote:
>>>> On Tue, Apr 14, 2015 at 01:33:08PM +0200, Arnd Bergmann wrote:
>>>>> This is true, but all other drivers do the same for GENERIC_PHY at the
>>>>> moment. If this one gets changed, we should probably apply the same
>>>>> solution to all current users and fix them consistently.
>>>>>
>>>>> We can do one of these two:
>>>>>
>>>>> a) make sure that the framework has 'static inline' stubs that let you
>>>>> build all drivers using it when the framework itself is disabled.
>>>>
>>>> Yes, please do that.
>>>>
>>>>> b) change the drivers using it to 'depends on', and make GENERIC_PHY
>>>>> itself a hidden option without a Kconfig prompt.
>>>>
>>>> Then how could GENERIC_PHY ever get set?
>>>
>>> Right now, every driver that provides a phy uses 'select GENERIC_PHY',
>>> and they would have to keep doing that. This is not unlike what we
>>> do for other silent symbols like MFD_CORE, REGMAP_I2C, or PINCTRL,
>>> and it's not as problematic as 'select' on a user-visible option,
>>> or (worst) mixing 'select' and 'depends on'.
>>
>> Ok, that would make more sense, but it would be good for the PHY
>> maintainer to agree to it as well :)
>
> looking at [1], we should use select only for non-visible symbols and
> for symbols with no dependencies. As such GENERIC_PHY is not dependent
> on other symbols but for now it is "visible".
>
> phy-core has all the stubs already implemented in
> include/linux/phy/phy.h. So removing select GENERIC_PHY shouldn't be a
> problem. But then it might break a few platforms where GENERIC_PHY is
> indirectly enabled by selecting the config of the driver (using default
> defconfigs in arch/arm/configs).
>
> The simplest thing would be to make GENERIC_PHY an invisible option?
>
> [1] ->
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/kbuild/kconfig-language.txt#n111
>
Kishon,removing select GENERIC_PHY also breaks the builds for certain
architectures (i386 and x84_64). Is the consensus to leave the select
but make GENERIC_PHY a invisible option? Thanks
>
> Thanks
> Kishon
On Tuesday 14 April 2015 11:05:35 Arun Ramamurthy wrote:
> >
> > [1] ->
> > https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/kbuild/kconfig-language.txt#n111
> >
> Kishon,removing select GENERIC_PHY also breaks the builds for certain
> architectures (i386 and x84_64). Is the consensus to leave the select
> but make GENERIC_PHY a invisible option? Thanks
I think the best solution is
- make GENERIC_PHY a silent option
- change PHY_RCAR_GEN2 to use 'select' instead of 'depends on', so
it will still work when all other phy drivers are disabled
- change the non-phy drivers that select GENERIC_PHY to either
use 'depends on' or no explicit dependency in case they are
still functional without the API. Note that
drivers/pinctrl/pinctrl-tegra-xusb.c is a phy provider as well,
not a consumer, despite being outside of drivers/phy.
Arnd
On Tue, Apr 14, 2015 at 3:40 AM, Arun Ramamurthy
<[email protected]> wrote:
> Getting phys by index instead of phy names so that we do
> not have to create a naming scheme when multiple phys
> are present
>
> Signed-off-by: Arun Ramamurthy <[email protected]>
> Reviewed-by: Ray Jui <[email protected]>
> Reviewed-by: Scott Branden <[email protected]>
> ---
> drivers/usb/host/Kconfig | 1 +
> drivers/usb/host/ehci-platform.c | 70 ++++++++++++++--------------------------
> 2 files changed, 26 insertions(+), 45 deletions(-)
>
> diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
> index 5ad60e4..563f22d 100644
> --- a/drivers/usb/host/Kconfig
> +++ b/drivers/usb/host/Kconfig
> @@ -284,6 +284,7 @@ config USB_EHCI_ATH79
>
> config USB_EHCI_HCD_PLATFORM
> tristate "Generic EHCI driver for a platform device"
> + select GENERIC_PHY
> default n
> ---help---
> Adds an EHCI host driver for a generic platform device, which
> diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
> index d8a75a5..a7563b9 100644
> --- a/drivers/usb/host/ehci-platform.c
> +++ b/drivers/usb/host/ehci-platform.c
> @@ -88,15 +88,13 @@ static int ehci_platform_power_on(struct platform_device *dev)
> }
>
> for (phy_num = 0; phy_num < priv->num_phys; phy_num++) {
> - if (priv->phys[phy_num]) {
> - ret = phy_init(priv->phys[phy_num]);
> - if (ret)
> - goto err_exit_phy;
> - ret = phy_power_on(priv->phys[phy_num]);
> - if (ret) {
> - phy_exit(priv->phys[phy_num]);
> - goto err_exit_phy;
> - }
> + ret = phy_init(priv->phys[phy_num]);
> + if (ret)
> + goto err_exit_phy;
Jumping to err_exit_phy will perform phy_power_off also which is not
required as you are are powering on after phy_init. Wrong level
jumping
~Rajeev
> + ret = phy_power_on(priv->phys[phy_num]);
> + if (ret) {
> + phy_exit(priv->phys[phy_num]);
> + goto err_exit_phy;
> }
> }
>
> @@ -104,10 +102,8 @@ static int ehci_platform_power_on(struct platform_device *dev)
>
> err_exit_phy:
> while (--phy_num >= 0) {
> - if (priv->phys[phy_num]) {
> - phy_power_off(priv->phys[phy_num]);
> - phy_exit(priv->phys[phy_num]);
> - }
> + phy_power_off(priv->phys[phy_num]);
> + phy_exit(priv->phys[phy_num]);
> }
> err_disable_clks:
> while (--clk >= 0)
> @@ -123,10 +119,8 @@ static void ehci_platform_power_off(struct platform_device *dev)
> int clk, phy_num;
>
> for (phy_num = 0; phy_num < priv->num_phys; phy_num++) {
> - if (priv->phys[phy_num]) {
> - phy_power_off(priv->phys[phy_num]);
> - phy_exit(priv->phys[phy_num]);
> - }
> + phy_power_off(priv->phys[phy_num]);
> + phy_exit(priv->phys[phy_num]);
> }
>
> for (clk = EHCI_MAX_CLKS - 1; clk >= 0; clk--)
> @@ -154,7 +148,6 @@ static int ehci_platform_probe(struct platform_device *dev)
> struct usb_ehci_pdata *pdata = dev_get_platdata(&dev->dev);
> struct ehci_platform_priv *priv;
> struct ehci_hcd *ehci;
> - const char *phy_name;
> int err, irq, phy_num, clk = 0;
>
> if (usb_disabled())
> @@ -204,36 +197,23 @@ static int ehci_platform_probe(struct platform_device *dev)
>
> priv->num_phys = of_count_phandle_with_args(dev->dev.of_node,
> "phys", "#phy-cells");
> - priv->num_phys = priv->num_phys > 0 ? priv->num_phys : 1;
>
> - priv->phys = devm_kcalloc(&dev->dev, priv->num_phys,
> - sizeof(struct phy *), GFP_KERNEL);
> - if (!priv->phys)
> - return -ENOMEM;
> + if (priv->num_phys > 0) {
> + priv->phys = devm_kcalloc(&dev->dev, priv->num_phys,
> + sizeof(struct phy *), GFP_KERNEL);
> + if (!priv->phys)
> + return -ENOMEM;
> + } else
> + priv->num_phys = 0;
>
> for (phy_num = 0; phy_num < priv->num_phys; phy_num++) {
> - err = of_property_read_string_index(
> - dev->dev.of_node,
> - "phy-names", phy_num,
> - &phy_name);
> -
> - if (err < 0) {
> - if (priv->num_phys > 1) {
> - dev_err(&dev->dev, "phy-names not provided");
> - goto err_put_hcd;
> - } else
> - phy_name = "usb";
> - }
> -
> - priv->phys[phy_num] = devm_phy_get(&dev->dev,
> - phy_name);
> - if (IS_ERR(priv->phys[phy_num])) {
> - err = PTR_ERR(priv->phys[phy_num]);
> - if ((priv->num_phys > 1) ||
> - (err == -EPROBE_DEFER))
> - goto err_put_hcd;
> - priv->phys[phy_num] = NULL;
> - }
> + priv->phys[phy_num] = devm_of_phy_get_by_index(&dev->dev
> + , dev->dev.of_node
> + , phy_num);
> + if (IS_ERR(priv->phys[phy_num])) {
> + err = PTR_ERR(priv->phys[phy_num]);
> + goto err_put_hcd;
> + }
> }
>
> for (clk = 0; clk < EHCI_MAX_CLKS; clk++) {
> --
> 2.3.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi,
On Tuesday 14 April 2015 03:40 AM, Arun Ramamurthy wrote:
> Some generic drivers, such as ehci, may use multiple phys and for such
> drivers referencing phy(s) by name(s) does not make sense. Instead of
> inventing new naming schemes and using custom code to iterate through them,
> such drivers are better of using nameless phy bindings and using this newly
> introduced API to iterate through them.
>
> Signed-off-by: Arun Ramamurthy <[email protected]>
> Reviewed-by: Ray Jui <[email protected]>
> Reviewed-by: Scott Branden <[email protected]>
> ---
> drivers/phy/phy-core.c | 32 ++++++++++++++++++++++++++++++++
> include/linux/phy/phy.h | 2 ++
> 2 files changed, 34 insertions(+)
>
> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> index 3791838..964a84d 100644
> --- a/drivers/phy/phy-core.c
> +++ b/drivers/phy/phy-core.c
> @@ -623,6 +623,38 @@ struct phy *devm_of_phy_get(struct device *dev, struct device_node *np,
> EXPORT_SYMBOL_GPL(devm_of_phy_get);
>
> /**
> + * devm_of_phy_get_by_index() - lookup and obtain a reference to a phy by index.
> + * @dev: device that requests this phy
> + * @np: node containing the phy
> + * @index: index of the phy
> + *
> + * Gets the phy using _of_phy_get(), and associates a device with it using
> + * devres. On driver detach, release function is invoked on the devres data,
> + * then, devres data is freed.
> + *
> + */
> +struct phy *devm_of_phy_get_by_index(struct device *dev, struct device_node *np,
> + int index)
> +{
> + struct phy **ptr, *phy;
> +
> + ptr = devres_alloc(devm_phy_release, sizeof(*ptr), GFP_KERNEL);
> + if (!ptr)
> + return ERR_PTR(-ENOMEM);
> +
> + phy = _of_phy_get(np, index);
> + if (!IS_ERR(phy)) {
> + *ptr = phy;
> + devres_add(dev, ptr);
> + } else {
> + devres_free(ptr);
> + }
> +
> + return phy;
> +}
> +EXPORT_SYMBOL_GPL(devm_of_phy_get_by_index);
> +
> +/**
> * phy_create() - create a new phy
> * @dev: device that is creating the new phy
> * @node: device node of the phy
> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> index a0197fa..ae2ffaf 100644
> --- a/include/linux/phy/phy.h
> +++ b/include/linux/phy/phy.h
> @@ -133,6 +133,8 @@ struct phy *devm_phy_get(struct device *dev, const char *string);
> struct phy *devm_phy_optional_get(struct device *dev, const char *string);
> struct phy *devm_of_phy_get(struct device *dev, struct device_node *np,
> const char *con_id);
> +struct phy *devm_of_phy_get_by_index(struct device *dev, struct device_node *np,
> + int index);
Add stubs for this function too. Also update the Documentation/phy.txt.
Thanks
Kishon
On Wed, 15 Apr 2015, rajeev kumar wrote:
> > @@ -88,15 +88,13 @@ static int ehci_platform_power_on(struct platform_device *dev)
> > }
> >
> > for (phy_num = 0; phy_num < priv->num_phys; phy_num++) {
> > - if (priv->phys[phy_num]) {
> > - ret = phy_init(priv->phys[phy_num]);
> > - if (ret)
> > - goto err_exit_phy;
> > - ret = phy_power_on(priv->phys[phy_num]);
> > - if (ret) {
> > - phy_exit(priv->phys[phy_num]);
> > - goto err_exit_phy;
> > - }
> > + ret = phy_init(priv->phys[phy_num]);
> > + if (ret)
> > + goto err_exit_phy;
>
> Jumping to err_exit_phy will perform phy_power_off also which is not
> required as you are are powering on after phy_init. Wrong level
> jumping
Look again, and this time pay more attention to the value of phy_num.
Alan Stern
> ~Rajeev
>
> > + ret = phy_power_on(priv->phys[phy_num]);
> > + if (ret) {
> > + phy_exit(priv->phys[phy_num]);
> > + goto err_exit_phy;
> > }
> > }
> >
> > @@ -104,10 +102,8 @@ static int ehci_platform_power_on(struct platform_device *dev)
> >
> > err_exit_phy:
> > while (--phy_num >= 0) {
> > - if (priv->phys[phy_num]) {
> > - phy_power_off(priv->phys[phy_num]);
> > - phy_exit(priv->phys[phy_num]);
> > - }
> > + phy_power_off(priv->phys[phy_num]);
> > + phy_exit(priv->phys[phy_num]);
> > }
On Wed, Apr 15, 2015 at 8:06 PM, Alan Stern <[email protected]> wrote:
> On Wed, 15 Apr 2015, rajeev kumar wrote:
>
>> > @@ -88,15 +88,13 @@ static int ehci_platform_power_on(struct platform_device *dev)
>> > }
>> >
>> > for (phy_num = 0; phy_num < priv->num_phys; phy_num++) {
>> > - if (priv->phys[phy_num]) {
>> > - ret = phy_init(priv->phys[phy_num]);
>> > - if (ret)
>> > - goto err_exit_phy;
>> > - ret = phy_power_on(priv->phys[phy_num]);
>> > - if (ret) {
>> > - phy_exit(priv->phys[phy_num]);
>> > - goto err_exit_phy;
>> > - }
>> > + ret = phy_init(priv->phys[phy_num]);
>> > + if (ret)
>> > + goto err_exit_phy;
>>
>> Jumping to err_exit_phy will perform phy_power_off also which is not
>> required as you are are powering on after phy_init. Wrong level
>> jumping
>
> Look again, and this time pay more attention to the value of phy_num.
>
> Alan Stern
MIssed it , Thanks for the pointer.
~Rajeev
>
>> ~Rajeev
>>
>> > + ret = phy_power_on(priv->phys[phy_num]);
>> > + if (ret) {
>> > + phy_exit(priv->phys[phy_num]);
>> > + goto err_exit_phy;
>> > }
>> > }
>> >
>> > @@ -104,10 +102,8 @@ static int ehci_platform_power_on(struct platform_device *dev)
>> >
>> > err_exit_phy:
>> > while (--phy_num >= 0) {
>> > - if (priv->phys[phy_num]) {
>> > - phy_power_off(priv->phys[phy_num]);
>> > - phy_exit(priv->phys[phy_num]);
>> > - }
>> > + phy_power_off(priv->phys[phy_num]);
>> > + phy_exit(priv->phys[phy_num]);
>> > }
>
>
On Mon, Apr 13, 2015 at 03:10:45PM -0700, Arun Ramamurthy wrote:
> Some generic drivers, such as ehci, may use multiple phys and for such
> drivers referencing phy(s) by name(s) does not make sense. Instead of
> inventing new naming schemes and using custom code to iterate through them,
> such drivers are better of using nameless phy bindings and using this newly
> introduced API to iterate through them.
>
> Signed-off-by: Arun Ramamurthy <[email protected]>
> Reviewed-by: Ray Jui <[email protected]>
> Reviewed-by: Scott Branden <[email protected]>
> ---
> drivers/phy/phy-core.c | 32 ++++++++++++++++++++++++++++++++
> include/linux/phy/phy.h | 2 ++
> 2 files changed, 34 insertions(+)
>
> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
> index 3791838..964a84d 100644
> --- a/drivers/phy/phy-core.c
> +++ b/drivers/phy/phy-core.c
> @@ -623,6 +623,38 @@ struct phy *devm_of_phy_get(struct device *dev, struct device_node *np,
> EXPORT_SYMBOL_GPL(devm_of_phy_get);
>
> /**
> + * devm_of_phy_get_by_index() - lookup and obtain a reference to a phy by index.
> + * @dev: device that requests this phy
> + * @np: node containing the phy
> + * @index: index of the phy
> + *
> + * Gets the phy using _of_phy_get(), and associates a device with it using
> + * devres. On driver detach, release function is invoked on the devres data,
> + * then, devres data is freed.
> + *
> + */
> +struct phy *devm_of_phy_get_by_index(struct device *dev, struct device_node *np,
> + int index)
> +{
> + struct phy **ptr, *phy;
> +
> + ptr = devres_alloc(devm_phy_release, sizeof(*ptr), GFP_KERNEL);
> + if (!ptr)
> + return ERR_PTR(-ENOMEM);
> +
> + phy = _of_phy_get(np, index);
> + if (!IS_ERR(phy)) {
> + *ptr = phy;
> + devres_add(dev, ptr);
> + } else {
> + devres_free(ptr);
> + }
> +
> + return phy;
> +}
> +EXPORT_SYMBOL_GPL(devm_of_phy_get_by_index);
> +
> +/**
> * phy_create() - create a new phy
> * @dev: device that is creating the new phy
> * @node: device node of the phy
> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
> index a0197fa..ae2ffaf 100644
> --- a/include/linux/phy/phy.h
> +++ b/include/linux/phy/phy.h
> @@ -133,6 +133,8 @@ struct phy *devm_phy_get(struct device *dev, const char *string);
> struct phy *devm_phy_optional_get(struct device *dev, const char *string);
> struct phy *devm_of_phy_get(struct device *dev, struct device_node *np,
> const char *con_id);
> +struct phy *devm_of_phy_get_by_index(struct device *dev, struct device_node *np,
> + int index);
You may need to add an implementation if CONFIG_GENERIC_PHY is not enabled.
> void phy_put(struct phy *phy);
> void devm_phy_put(struct device *dev, struct phy *phy);
> struct phy *of_phy_get(struct device_node *np, const char *con_id);
> --
> 2.3.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Best Regards,
Peter Chen
On 15-04-15 02:59 AM, Kishon Vijay Abraham I wrote:
> Hi,
>
> On Tuesday 14 April 2015 03:40 AM, Arun Ramamurthy wrote:
>> Some generic drivers, such as ehci, may use multiple phys and for such
>> drivers referencing phy(s) by name(s) does not make sense. Instead of
>> inventing new naming schemes and using custom code to iterate through
>> them,
>> such drivers are better of using nameless phy bindings and using this
>> newly
>> introduced API to iterate through them.
>>
>> Signed-off-by: Arun Ramamurthy <[email protected]>
>> Reviewed-by: Ray Jui <[email protected]>
>> Reviewed-by: Scott Branden <[email protected]>
>> ---
>> drivers/phy/phy-core.c | 32 ++++++++++++++++++++++++++++++++
>> include/linux/phy/phy.h | 2 ++
>> 2 files changed, 34 insertions(+)
>>
>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
>> index 3791838..964a84d 100644
>> --- a/drivers/phy/phy-core.c
>> +++ b/drivers/phy/phy-core.c
>> @@ -623,6 +623,38 @@ struct phy *devm_of_phy_get(struct device *dev,
>> struct device_node *np,
>> EXPORT_SYMBOL_GPL(devm_of_phy_get);
>>
>> /**
>> + * devm_of_phy_get_by_index() - lookup and obtain a reference to a
>> phy by index.
>> + * @dev: device that requests this phy
>> + * @np: node containing the phy
>> + * @index: index of the phy
>> + *
>> + * Gets the phy using _of_phy_get(), and associates a device with it
>> using
>> + * devres. On driver detach, release function is invoked on the
>> devres data,
>> + * then, devres data is freed.
>> + *
>> + */
>> +struct phy *devm_of_phy_get_by_index(struct device *dev, struct
>> device_node *np,
>> + int index)
>> +{
>> + struct phy **ptr, *phy;
>> +
>> + ptr = devres_alloc(devm_phy_release, sizeof(*ptr), GFP_KERNEL);
>> + if (!ptr)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + phy = _of_phy_get(np, index);
>> + if (!IS_ERR(phy)) {
>> + *ptr = phy;
>> + devres_add(dev, ptr);
>> + } else {
>> + devres_free(ptr);
>> + }
>> +
>> + return phy;
>> +}
>> +EXPORT_SYMBOL_GPL(devm_of_phy_get_by_index);
>> +
>> +/**
>> * phy_create() - create a new phy
>> * @dev: device that is creating the new phy
>> * @node: device node of the phy
>> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
>> index a0197fa..ae2ffaf 100644
>> --- a/include/linux/phy/phy.h
>> +++ b/include/linux/phy/phy.h
>> @@ -133,6 +133,8 @@ struct phy *devm_phy_get(struct device *dev, const
>> char *string);
>> struct phy *devm_phy_optional_get(struct device *dev, const char
>> *string);
>> struct phy *devm_of_phy_get(struct device *dev, struct device_node *np,
>> const char *con_id);
>> +struct phy *devm_of_phy_get_by_index(struct device *dev, struct
>> device_node *np,
>> + int index);
>
> Add stubs for this function too. Also update the Documentation/phy.txt.
>
Kishon, I have added stubs for this function in my next patch set.
However I am still unclear on whether I need to make GENERIC_PHY an
invisible option or change my "select" to "depend" ? It seems like there
was no consensus on this? Do you have any final thoughts before i send
out the next patch set? Thanks
> Thanks
> Kishon
Arnd,
On Wednesday 15 April 2015 03:17 AM, Arnd Bergmann wrote:
> On Tuesday 14 April 2015 11:05:35 Arun Ramamurthy wrote:
>>>
>>> [1] ->
>>> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/kbuild/kconfig-language.txt#n111
>>>
>> Kishon,removing select GENERIC_PHY also breaks the builds for certain
>> architectures (i386 and x84_64). Is the consensus to leave the select
>> but make GENERIC_PHY a invisible option? Thanks
>
> I think the best solution is
>
> - make GENERIC_PHY a silent option
> - change PHY_RCAR_GEN2 to use 'select' instead of 'depends on', so
> it will still work when all other phy drivers are disabled
> - change the non-phy drivers that select GENERIC_PHY to either
> use 'depends on' or no explicit dependency in case they are
> still functional without the API. Note that
> drivers/pinctrl/pinctrl-tegra-xusb.c is a phy provider as well,
> not a consumer, despite being outside of drivers/phy.
makes sense to me.
Thanks
Kishon
Hi,
On Tuesday 21 April 2015 01:49 AM, Arun Ramamurthy wrote:
>
>
> On 15-04-15 02:59 AM, Kishon Vijay Abraham I wrote:
>> Hi,
>>
>> On Tuesday 14 April 2015 03:40 AM, Arun Ramamurthy wrote:
>>> Some generic drivers, such as ehci, may use multiple phys and for such
>>> drivers referencing phy(s) by name(s) does not make sense. Instead of
>>> inventing new naming schemes and using custom code to iterate through
>>> them,
>>> such drivers are better of using nameless phy bindings and using this
>>> newly
>>> introduced API to iterate through them.
>>>
>>> Signed-off-by: Arun Ramamurthy <[email protected]>
>>> Reviewed-by: Ray Jui <[email protected]>
>>> Reviewed-by: Scott Branden <[email protected]>
>>> ---
>>> drivers/phy/phy-core.c | 32 ++++++++++++++++++++++++++++++++
>>> include/linux/phy/phy.h | 2 ++
>>> 2 files changed, 34 insertions(+)
>>>
>>> diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
>>> index 3791838..964a84d 100644
>>> --- a/drivers/phy/phy-core.c
>>> +++ b/drivers/phy/phy-core.c
>>> @@ -623,6 +623,38 @@ struct phy *devm_of_phy_get(struct device *dev,
>>> struct device_node *np,
>>> EXPORT_SYMBOL_GPL(devm_of_phy_get);
>>>
>>> /**
>>> + * devm_of_phy_get_by_index() - lookup and obtain a reference to a
>>> phy by index.
>>> + * @dev: device that requests this phy
>>> + * @np: node containing the phy
>>> + * @index: index of the phy
>>> + *
>>> + * Gets the phy using _of_phy_get(), and associates a device with it
>>> using
>>> + * devres. On driver detach, release function is invoked on the
>>> devres data,
>>> + * then, devres data is freed.
>>> + *
>>> + */
>>> +struct phy *devm_of_phy_get_by_index(struct device *dev, struct
>>> device_node *np,
>>> + int index)
>>> +{
>>> + struct phy **ptr, *phy;
>>> +
>>> + ptr = devres_alloc(devm_phy_release, sizeof(*ptr), GFP_KERNEL);
>>> + if (!ptr)
>>> + return ERR_PTR(-ENOMEM);
>>> +
>>> + phy = _of_phy_get(np, index);
>>> + if (!IS_ERR(phy)) {
>>> + *ptr = phy;
>>> + devres_add(dev, ptr);
>>> + } else {
>>> + devres_free(ptr);
>>> + }
>>> +
>>> + return phy;
>>> +}
>>> +EXPORT_SYMBOL_GPL(devm_of_phy_get_by_index);
>>> +
>>> +/**
>>> * phy_create() - create a new phy
>>> * @dev: device that is creating the new phy
>>> * @node: device node of the phy
>>> diff --git a/include/linux/phy/phy.h b/include/linux/phy/phy.h
>>> index a0197fa..ae2ffaf 100644
>>> --- a/include/linux/phy/phy.h
>>> +++ b/include/linux/phy/phy.h
>>> @@ -133,6 +133,8 @@ struct phy *devm_phy_get(struct device *dev, const
>>> char *string);
>>> struct phy *devm_phy_optional_get(struct device *dev, const char
>>> *string);
>>> struct phy *devm_of_phy_get(struct device *dev, struct device_node *np,
>>> const char *con_id);
>>> +struct phy *devm_of_phy_get_by_index(struct device *dev, struct
>>> device_node *np,
>>> + int index);
>>
>> Add stubs for this function too. Also update the Documentation/phy.txt.
>>
> Kishon, I have added stubs for this function in my next patch set.
> However I am still unclear on whether I need to make GENERIC_PHY an
> invisible option or change my "select" to "depend" ? It seems like there
> was no consensus on this? Do you have any final thoughts before i send
> out the next patch set? Thanks
You can follow Arnd's suggestion. You can have a separate patch to change the
GENERIC_PHY to invisible option and change existing PHY drivers to select
GENERIC_PHY.
Non-PHY drivers can either use depends on or have no explicit dependency if the
PHY is optional for that controller.
Thanks
Kishon