| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20160217152145.44ae8676@tock>
Date: Wed, 17 Feb 2016 15:21:45 +0100
From: Alban <albeu@...e.fr>
To: Kishon Vijay Abraham I <kishon@...com>
Cc: Aban Bedel <albeu@...e.fr>, <linux-kernel@...r.kernel.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"David S. Miller" <davem@...emloft.net>,
Mauro Carvalho Chehab <mchehab@....samsung.com>,
Joe Perches <joe@...ches.com>, Jiri Slaby <jslaby@...e.com>
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@...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@...e.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@...e.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
Powered by blists - more mailing lists