From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755028AbcIAIDV (ORCPT ); Thu, 1 Sep 2016 04:03:21 -0400 Received: from mail-pa0-f47.google.com ([209.85.220.47]:36095 "EHLO mail-pa0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754950AbcIAIDJ (ORCPT ); Thu, 1 Sep 2016 04:03:09 -0400 Subject: Re: [PATCH v6 2/8] power: add power sequence library To: Peter Chen , gregkh@linuxfoundation.org, stern@rowland.harvard.edu, ulf.hansson@linaro.org, broonie@kernel.org, sre@kernel.org, robh+dt@kernel.org, shawnguo@kernel.org, dbaryshkov@gmail.com, dwmw3@infradead.org References: <1471252398-957-1-git-send-email-peter.chen@nxp.com> <1471252398-957-3-git-send-email-peter.chen@nxp.com> Cc: mark.rutland@arm.com, devicetree@vger.kernel.org, k.kozlowski@samsung.com, stephen.boyd@linaro.org, oscar@naiandei.net, arnd@arndb.de, pawel.moll@arm.com, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, s.hauer@pengutronix.de, linux-usb@vger.kernel.org, mail@maciej.szmigiero.name, troy.kisky@boundarydevices.com, stillcompiling@gmail.com, p.zabel@pengutronix.de, festevam@gmail.com, mka@chromium.org, linux-arm-kernel@lists.infradead.org From: Vaibhav Hiremath Message-ID: <4a08d6f8-1c6a-fa1b-0dfe-657afa4e8a90@linaro.org> Date: Thu, 1 Sep 2016 13:32:56 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <1471252398-957-3-git-send-email-peter.chen@nxp.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Monday 15 August 2016 02:43 PM, Peter Chen wrote: > We have an well-known problem that the device needs to do some power > sequence before it can be recognized by related host, the typical > example like hard-wired mmc devices and usb devices. > > This power sequence is hard to be described at device tree and handled by > related host driver, so we have created a common power sequence > library to cover this requirement. The core code has supplied > some common helpers for host driver, and individual power sequence > libraries handle kinds of power sequence for devices. > > pwrseq_generic is intended for general purpose of power sequence, which > handles gpios and clocks currently, and can cover regulator and pinctrl > in future. The host driver calls pwrseq_alloc_generic to create > an generic pwrseq instance. > > Signed-off-by: Peter Chen > Tested-by Joshua Clayton > Reviewed-by: Matthias Kaehlcke > Tested-by: Matthias Kaehlcke > --- > MAINTAINERS | 9 ++ > drivers/power/Kconfig | 1 + > drivers/power/Makefile | 1 + > drivers/power/pwrseq/Kconfig | 20 ++++ > drivers/power/pwrseq/Makefile | 2 + > drivers/power/pwrseq/core.c | 62 +++++++++++++ > drivers/power/pwrseq/pwrseq_generic.c | 168 ++++++++++++++++++++++++++++++++++ > include/linux/power/pwrseq.h | 47 ++++++++++ > 8 files changed, 310 insertions(+) > create mode 100644 drivers/power/pwrseq/Kconfig > create mode 100644 drivers/power/pwrseq/Makefile > create mode 100644 drivers/power/pwrseq/core.c > create mode 100644 drivers/power/pwrseq/pwrseq_generic.c > create mode 100644 include/linux/power/pwrseq.h > > diff --git a/MAINTAINERS b/MAINTAINERS > index 1ae6c84..407254b 100644 > --- a/MAINTAINERS > +++ b/MAINTAINERS > @@ -9283,6 +9283,15 @@ F: include/linux/pm_* > F: include/linux/powercap.h > F: drivers/powercap/ > > +POWER SEQUENCE LIBRARY > +M: Peter Chen > +T: git git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git > +L: linux-pm@vger.kernel.org > +S: Maintained > +F: Documentation/devicetree/bindings/power/pwrseq/ > +F: drivers/power/pwrseq/ > +F: include/linux/power/pwrseq.h/ > + > POWER SUPPLY CLASS/SUBSYSTEM and DRIVERS > M: Sebastian Reichel > M: Dmitry Eremin-Solenikov > diff --git a/drivers/power/Kconfig b/drivers/power/Kconfig > index acd4a15..f6aa4fd 100644 > --- a/drivers/power/Kconfig > +++ b/drivers/power/Kconfig > @@ -515,3 +515,4 @@ endif # POWER_SUPPLY > > source "drivers/power/reset/Kconfig" > source "drivers/power/avs/Kconfig" > +source "drivers/power/pwrseq/Kconfig" > diff --git a/drivers/power/Makefile b/drivers/power/Makefile > index e46b75d..4ed2e12 100644 > --- a/drivers/power/Makefile > +++ b/drivers/power/Makefile > @@ -74,3 +74,4 @@ obj-$(CONFIG_CHARGER_TPS65217) += tps65217_charger.o > obj-$(CONFIG_POWER_RESET) += reset/ > obj-$(CONFIG_AXP288_FUEL_GAUGE) += axp288_fuel_gauge.o > obj-$(CONFIG_AXP288_CHARGER) += axp288_charger.o > +obj-$(CONFIG_POWER_SEQUENCE) += pwrseq/ > diff --git a/drivers/power/pwrseq/Kconfig b/drivers/power/pwrseq/Kconfig > new file mode 100644 > index 0000000..188729e > --- /dev/null > +++ b/drivers/power/pwrseq/Kconfig > @@ -0,0 +1,20 @@ > +# > +# Power Sequence library > +# > + > +config POWER_SEQUENCE > + bool > + > +menu "Power Sequence Support" > + > +config PWRSEQ_GENERIC > + bool "Generic power sequence control" > + depends on OF > + select POWER_SEQUENCE > + help > + It is used for drivers which needs to do power sequence > + (eg, turn on clock, toggle reset gpio) before the related > + devices can be found by hardware. This generic one can be > + used for common power sequence control. > + > +endmenu > diff --git a/drivers/power/pwrseq/Makefile b/drivers/power/pwrseq/Makefile > new file mode 100644 > index 0000000..ad82389 > --- /dev/null > +++ b/drivers/power/pwrseq/Makefile > @@ -0,0 +1,2 @@ > +obj-$(CONFIG_POWER_SEQUENCE) += core.o > +obj-$(CONFIG_PWRSEQ_GENERIC) += pwrseq_generic.o > diff --git a/drivers/power/pwrseq/core.c b/drivers/power/pwrseq/core.c > new file mode 100644 > index 0000000..dcf96c4 > --- /dev/null > +++ b/drivers/power/pwrseq/core.c > @@ -0,0 +1,62 @@ > +/* > + * core.c power sequence core file > + * > + * Copyright (C) 2016 Freescale Semiconductor, Inc. > + * Author: Peter Chen > + * > + * This program is free software: you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 of > + * the License as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program. If not, see . > + */ > + > +#include > + > +static DEFINE_MUTEX(pwrseq_list_mutex); > +static LIST_HEAD(pwrseq_list); > + > +int pwrseq_get(struct device_node *np, struct pwrseq *p) > +{ > + if (p && p->get) > + return p->get(np, p); > + > + return -ENOTSUPP; > +} > +EXPORT_SYMBOL(pwrseq_get); > + > +int pwrseq_on(struct device_node *np, struct pwrseq *p) > +{ > + if (p && p->on) > + return p->on(np, p); > + > + return -ENOTSUPP; > +} > +EXPORT_SYMBOL(pwrseq_on); > + > +void pwrseq_off(struct pwrseq *p) > +{ > + if (p && p->off) > + p->off(p); > +} > +EXPORT_SYMBOL(pwrseq_off); > + > +void pwrseq_put(struct pwrseq *p) > +{ > + if (p && p->put) > + p->put(p); > +} > +EXPORT_SYMBOL(pwrseq_put); > + > +void pwrseq_free(struct pwrseq *p) > +{ > + if (p && p->free) > + p->free(p); > +} > +EXPORT_SYMBOL(pwrseq_free); > diff --git a/drivers/power/pwrseq/pwrseq_generic.c b/drivers/power/pwrseq/pwrseq_generic.c > new file mode 100644 > index 0000000..8af626f > --- /dev/null > +++ b/drivers/power/pwrseq/pwrseq_generic.c > @@ -0,0 +1,168 @@ > +/* > + * pwrseq_generic.c Generic power sequence handling > + * > + * Copyright (C) 2016 Freescale Semiconductor, Inc. > + * Author: Peter Chen > + * > + * This program is free software: you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 of > + * the License as published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + * > + * You should have received a copy of the GNU General Public License > + * along with this program. If not, see . > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include > + > +struct pwrseq_generic { > + struct pwrseq pwrseq; > + struct gpio_desc *gpiod_reset; > + struct clk *clks[PWRSEQ_MAX_CLKS]; > +}; > + > +#define to_generic_pwrseq(p) container_of(p, struct pwrseq_generic, pwrseq) > + > +static void pwrseq_generic_free(struct pwrseq *pwrseq) > +{ > + struct pwrseq_generic *pwrseq_gen = to_generic_pwrseq(pwrseq); > + > + kfree(pwrseq_gen); > +} > + > +static void pwrseq_generic_put(struct pwrseq *pwrseq) > +{ > + struct pwrseq_generic *pwrseq_gen = to_generic_pwrseq(pwrseq); > + int clk; > + > + if (pwrseq_gen->gpiod_reset) > + gpiod_put(pwrseq_gen->gpiod_reset); > + > + for (clk = 0; clk < PWRSEQ_MAX_CLKS; clk++) > + clk_put(pwrseq_gen->clks[clk]); > +} > + > +static void pwrseq_generic_off(struct pwrseq *pwrseq) > +{ > + struct pwrseq_generic *pwrseq_gen = to_generic_pwrseq(pwrseq); > + int clk; > + > + for (clk = PWRSEQ_MAX_CLKS - 1; clk >= 0; clk--) > + clk_disable_unprepare(pwrseq_gen->clks[clk]); > +} > + > +static int pwrseq_generic_on(struct device_node *np, struct pwrseq *pwrseq) Ideally you shouldn't need device_node here at this stage. I expect to extract all the resource information in _get() itself. > +{ > + struct pwrseq_generic *pwrseq_gen = to_generic_pwrseq(pwrseq); > + int clk, ret = 0; > + struct gpio_desc *gpiod_reset = pwrseq_gen->gpiod_reset; > + > + for (clk = 0; clk < PWRSEQ_MAX_CLKS && pwrseq_gen->clks[clk]; clk++) { > + ret = clk_prepare_enable(pwrseq_gen->clks[clk]); > + if (ret) { > + pr_err("Can't enable clock on %s: %d\n", > + np->full_name, ret); > + goto err_disable_clks; > + } > + } > + > + if (gpiod_reset) { > + u32 duration_us = 50; Why initialize to 50 ?? > + > + of_property_read_u32(np, "reset-duration-us", > + &duration_us); > + if (duration_us <= 10) > + udelay(10); > + else > + usleep_range(duration_us, duration_us + 100); > + gpiod_set_value(gpiod_reset, 0); > + } > + > + return ret; > + > +err_disable_clks: > + while (--clk >= 0) > + clk_disable_unprepare(pwrseq_gen->clks[clk]); > + > + return ret; > +} > + > +static int pwrseq_generic_get(struct device_node *np, struct pwrseq *pwrseq) > +{ > + struct pwrseq_generic *pwrseq_gen = to_generic_pwrseq(pwrseq); > + enum of_gpio_flags flags; > + int reset_gpio, clk, ret = 0; > + > + for (clk = 0; clk < PWRSEQ_MAX_CLKS; clk++) { > + pwrseq_gen->clks[clk] = of_clk_get(np, clk); > + if (IS_ERR(pwrseq_gen->clks[clk])) { > + ret = PTR_ERR(pwrseq_gen->clks[clk]); > + if (ret == -EPROBE_DEFER) > + goto err_put_clks; > + pwrseq_gen->clks[clk] = NULL; > + break; Do we really want to continue here, even we failed to get the clock ? > + } > + } > + > + reset_gpio = of_get_named_gpio_flags(np, "reset-gpios", 0, &flags); > + if (gpio_is_valid(reset_gpio)) { > + unsigned long gpio_flags; > + > + if (flags & OF_GPIO_ACTIVE_LOW) > + gpio_flags = GPIOF_ACTIVE_LOW | GPIOF_OUT_INIT_LOW; > + else > + gpio_flags = GPIOF_OUT_INIT_HIGH; > + > + ret = gpio_request_one(reset_gpio, gpio_flags, > + "pwrseq-reset-gpios"); > + if (ret) > + goto err_put_clks; > + > + pwrseq_gen->gpiod_reset = gpio_to_desc(reset_gpio); > + } else { > + if (reset_gpio == -ENOENT) > + return 0; Why are we returning success here ? > + > + ret = reset_gpio; > + pr_err("Failed to get reset gpio on %s, err = %d\n", > + np->full_name, reset_gpio); > + goto err_put_clks; > + } > + > + return ret; > + > +err_put_clks: > + while (--clk >= 0) > + clk_put(pwrseq_gen->clks[clk]); > + return ret; > +} > + > +struct pwrseq *pwrseq_alloc_generic(void) > +{ > + struct pwrseq_generic *pwrseq_gen; > + > + pwrseq_gen = kzalloc(sizeof(*pwrseq_gen), GFP_KERNEL); > + if (!pwrseq_gen) > + return ERR_PTR(-ENOMEM); > + > + pwrseq_gen->pwrseq.get = pwrseq_generic_get; > + pwrseq_gen->pwrseq.on = pwrseq_generic_on; > + pwrseq_gen->pwrseq.off = pwrseq_generic_off; > + pwrseq_gen->pwrseq.put = pwrseq_generic_put; > + pwrseq_gen->pwrseq.free = pwrseq_generic_free; > + > + return &pwrseq_gen->pwrseq; > +} > +EXPORT_SYMBOL_GPL(pwrseq_alloc_generic); With recent discussion on postcore_initcall, we probably can merge _get() and _alloc() fns. Thanks, Vaibhav > diff --git a/include/linux/power/pwrseq.h b/include/linux/power/pwrseq.h > new file mode 100644 > index 0000000..ebb2280 > --- /dev/null > +++ b/include/linux/power/pwrseq.h > @@ -0,0 +1,47 @@ > +#ifndef __LINUX_PWRSEQ_H > +#define __LINUX_PWRSEQ_H > + > +#include > + > +#define PWRSEQ_MAX_CLKS 3 > + > +struct pwrseq { > + char *name; > + struct list_head node; > + int (*get)(struct device_node *np, struct pwrseq *p); > + int (*on)(struct device_node *np, struct pwrseq *p); > + void (*off)(struct pwrseq *p); > + void (*put)(struct pwrseq *p); > + void (*free)(struct pwrseq *p); > +}; > + > +#if IS_ENABLED(CONFIG_POWER_SEQUENCE) > +int pwrseq_get(struct device_node *np, struct pwrseq *p); > +int pwrseq_on(struct device_node *np, struct pwrseq *p); > +void pwrseq_off(struct pwrseq *p); > +void pwrseq_put(struct pwrseq *p); > +void pwrseq_free(struct pwrseq *p); > +#else > +static inline int pwrseq_get(struct device_node *np, struct pwrseq *p) > +{ > + return 0; > +} > +static inline int pwrseq_on(struct device_node *np, struct pwrseq *p) > +{ > + return 0; > +} > +static inline void pwrseq_off(struct pwrseq *p) {} > +static inline void pwrseq_put(struct pwrseq *p) {} > +static inline void pwrseq_free(struct pwrseq *p) {} > +#endif /* CONFIG_POWER_SEQUENCE */ > + > +#if IS_ENABLED(CONFIG_PWRSEQ_GENERIC) > +struct pwrseq *pwrseq_alloc_generic(void); > +#else > +static inline struct pwrseq *pwrseq_alloc_generic(void) > +{ > + return NULL; > +} > +#endif /* CONFIG_PWRSEQ_GENERIC */ > + > +#endif /* __LINUX_PWRSEQ_H */ -- Thanks, Vaibhav