Messages in this thread | | | Date | Wed, 17 Feb 2016 15:21:45 +0100 | From | Alban <> | Subject | Re: [PATCH v3 1/2] phy: Add a driver for simple phy |
| |
On Mon, 1 Feb 2016 17:12:42 +0530 Kishon Vijay Abraham I <kishon@ti.com> wrote:
> Hi, > > On Friday 29 January 2016 01:22 AM, Alban Bedel wrote: > > This driver is meant to take care of all trivial phys that don't need > > any special configuration, it just enable a regulator, a clock and > > deassert a reset. A public API is also included to allow re-using the > > code in other drivers. > > > > Signed-off-by: Alban Bedel <albeu@free.fr> > > --- > > please also include what changed from the previous versions here or in the > cover letter. > > drivers/phy/Kconfig | 12 +++ > > drivers/phy/Makefile | 1 + > > drivers/phy/phy-simple.c | 204 +++++++++++++++++++++++++++++++++++++++++++++ > > include/linux/phy/simple.h | 39 +++++++++ > > 4 files changed, 256 insertions(+) > > create mode 100644 drivers/phy/phy-simple.c > > create mode 100644 include/linux/phy/simple.h > > > > diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig > > index e7e117d..efbaee4 100644 > > --- a/drivers/phy/Kconfig > > +++ b/drivers/phy/Kconfig > > @@ -125,6 +125,18 @@ config PHY_RCAR_GEN3_USB2 > > help > > Support for USB 2.0 PHY found on Renesas R-Car generation 3 SoCs. > > > > +config PHY_SIMPLE > > + tristate > > + select GENERIC_PHY > > + help > > + > > +config PHY_SIMPLE_PDEV > > + tristate "Simple PHY driver" > > + select PHY_SIMPLE > > + help > > + A PHY driver for simple devices that only need a regulator, clock > > + and reset for power up and shutdown. > > + > > config OMAP_CONTROL_PHY > > tristate "OMAP CONTROL PHY Driver" > > depends on ARCH_OMAP2PLUS || COMPILE_TEST > > diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile > > index c80f09d..1cc7268 100644 > > --- a/drivers/phy/Makefile > > +++ b/drivers/phy/Makefile > > @@ -26,6 +26,7 @@ obj-$(CONFIG_PHY_EXYNOS5250_SATA) += phy-exynos5250-sata.o > > obj-$(CONFIG_PHY_HIX5HD2_SATA) += phy-hix5hd2-sata.o > > obj-$(CONFIG_PHY_HI6220_USB) += phy-hi6220-usb.o > > obj-$(CONFIG_PHY_MT65XX_USB3) += phy-mt65xx-usb3.o > > +obj-$(CONFIG_PHY_SIMPLE) += phy-simple.o > > obj-$(CONFIG_PHY_SUN4I_USB) += phy-sun4i-usb.o > > obj-$(CONFIG_PHY_SUN9I_USB) += phy-sun9i-usb.o > > obj-$(CONFIG_PHY_SAMSUNG_USB2) += phy-exynos-usb2.o > > diff --git a/drivers/phy/phy-simple.c b/drivers/phy/phy-simple.c > > new file mode 100644 > > index 0000000..013f846 > > --- /dev/null > > +++ b/drivers/phy/phy-simple.c > > @@ -0,0 +1,204 @@ > > +/* > > + * Copyright (C) 2015 Alban Bedel <albeu@free.fr> > > 2016?
I'll add 2016.
> > + * > > + * This program is free software; you can redistribute it and/or modify > > + * it under the terms of the GNU General Public License as published by > > + * the Free Software Foundation; either version 2 of the License, or > > + * (at your option) any later version. > > + */ > > + > > +#include <linux/module.h> > > +#include <linux/of_device.h> > > +#include <linux/phy/simple.h> > > +#include <linux/clk.h> > > +#include <linux/reset.h> > > + > > +int simple_phy_power_on(struct phy *phy) > > +{ > > + struct simple_phy *sphy = phy_get_drvdata(phy); > > + int err; > > + > > + if (sphy->regulator) { > > + err = regulator_enable(sphy->regulator); > > + if (err) > > + return err; > > + } > > phy_power_on already enables the regulator, so this might be redundant. A > simple PHY can have multiple regulators?
You are right, this can be dropped.
> Also this doesn't have reference count. I think we should try to reuse the > existing phy_* APIs for simple PHY.
I don't really get what you mean here. The phy core already count the enable/disable and only call the callbacks implemented here for the first enable and last disable. What should be done differently?
> > + > > + if (sphy->clk) { > > + err = clk_prepare_enable(sphy->clk); > > + if (err) > > + goto regulator_disable; > > + } > > how do we enable multiple clocks?
Currently you can't. However I don't think it would make much sense because if there is multiple clocks it start to be non-trivial. In such case you will often get dependencies between the clocks and/or power supplies. At this point you might as well write a dedicated driver.
> > + > > + if (sphy->reset) { > > + err = reset_control_deassert(sphy->reset); > > + if (err) > > + goto clock_disable; > > + } > > + > > + return 0; > > + > > +clock_disable: > > + if (sphy->clk) > > + clk_disable_unprepare(sphy->clk); > > +regulator_disable: > > + if (sphy->regulator) > > + WARN_ON(regulator_disable(sphy->regulator)); > > + > > + return err; > > +} > > +EXPORT_SYMBOL_GPL(simple_phy_power_on); > > + > > +int simple_phy_power_off(struct phy *phy) > > +{ > > + struct simple_phy *sphy = phy_get_drvdata(phy); > > + int err; > > + > > + if (sphy->reset) { > > + err = reset_control_assert(sphy->reset); > > + if (err) > > + return err; > > + } > > + > > + if (sphy->clk) > > + clk_disable_unprepare(sphy->clk); > > + > > + if (sphy->regulator) { > > + err = regulator_disable(sphy->regulator); > > + if (err) > > + goto clock_enable; > > + } > > + > > + return 0; > > + > > +clock_enable: > > + if (sphy->clk) > > + WARN_ON(clk_prepare_enable(sphy->clk)); > > + if (sphy->reset) > > + WARN_ON(reset_control_deassert(sphy->reset)); > > + > > + return err; > > +} > > +EXPORT_SYMBOL_GPL(simple_phy_power_off); > > + > > +static const struct phy_ops simple_phy_ops = { > > + .power_on = simple_phy_power_on, > > + .power_off = simple_phy_power_off, > > + .owner = THIS_MODULE, > > +}; > > + > > +struct phy *devm_simple_phy_create(struct device *dev, > > + const struct simple_phy_desc *desc, > > + struct simple_phy *sphy) > > +{ > > + struct phy *phy; > > + > > + if (!dev || !desc) > > + return ERR_PTR(-EINVAL); > > + > > + if (!sphy) { > > + sphy = devm_kzalloc(dev, sizeof(*sphy), GFP_KERNEL); > > + if (!sphy) > > + return ERR_PTR(-ENOMEM); > > + } > > + > > + if (!IS_ERR_OR_NULL(desc->regulator)) { > > + sphy->regulator = devm_regulator_get(dev, desc->regulator); > > + if (IS_ERR(sphy->regulator)) { > > + if (PTR_ERR(sphy->regulator) == -ENOENT) > > + sphy->regulator = NULL; > > + else > > + return ERR_PTR(PTR_ERR(sphy->regulator)); > > + } > > + } > > + > > + if (!IS_ERR(desc->clk)) { > > + sphy->clk = devm_clk_get(dev, desc->clk); > > + if (IS_ERR(sphy->clk)) { > > + if (PTR_ERR(sphy->clk) == -ENOENT) > > + sphy->clk = NULL; > > + else > > + return ERR_PTR(PTR_ERR(sphy->clk)); > > + } > > + } > > + > > + if (!IS_ERR(desc->reset)) { > > + sphy->reset = devm_reset_control_get(dev, desc->reset); > > + if (IS_ERR(sphy->reset)) { > > + int err = PTR_ERR(sphy->reset); > > + > > + if (err == -ENOENT || err == -ENOTSUPP) > > + sphy->reset = NULL; > > + else > > + return ERR_PTR(err); > > + } > > + } > > + > > + phy = devm_phy_create(dev, NULL, desc->ops ?: &simple_phy_ops); > > + if (IS_ERR(phy)) > > + return ERR_PTR(PTR_ERR(phy)); > > + > > + phy_set_drvdata(phy, sphy); > > + > > + return phy; > > +} > > +EXPORT_SYMBOL_GPL(devm_simple_phy_create); > > + > > +#ifdef CONFIG_PHY_SIMPLE_PDEV > > +#ifdef CONFIG_OF > > +/* Default config, no regulator, default clock and reset if any */ > > +static const struct simple_phy_desc simple_phy_default_desc = {}; > > + > > +static const struct of_device_id simple_phy_of_match[] = { > > + { .compatible = "simple-phy", .data = &simple_phy_default_desc }, > > the compatible string should be documented.
I'll add a patch with the binding to the next re-roll.
Alban
| |