linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Heiko Stuebner <heiko@sntech.de>
To: Peter Chen <peter.chen@nxp.com>
Cc: 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, k.kozlowski@samsung.com,
	linux-arm-kernel@lists.infradead.org, p.zabel@pengutronix.de,
	devicetree@vger.kernel.org, pawel.moll@arm.com,
	mark.rutland@arm.com, linux-usb@vger.kernel.org, arnd@arndb.de,
	s.hauer@pengutronix.de, mail@maciej.szmigiero.name,
	troy.kisky@boundarydevices.com, festevam@gmail.com,
	oscar@naiandei.net, stephen.boyd@linaro.org,
	linux-pm@vger.kernel.org, stillcompiling@gmail.com,
	linux-kernel@vger.kernel.org, mka@chromium.org,
	vaibhav.hiremath@linaro.org
Subject: Re: [PATCH v7 2/8] power: add power sequence library
Date: Wed, 12 Oct 2016 12:30:29 +0200	[thread overview]
Message-ID: <4691591.qnCGxrEkrZ@phil> (raw)
In-Reply-To: <1474342607-27512-3-git-send-email-peter.chen@nxp.com>

Hi,

Am Dienstag, 20. September 2016, 11:36:41 CEST schrieb Peter Chen:
> 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 just needs to call of_pwrseq_on/of_pwrseq_off
> if only one power sequence is needed, else call of_pwrseq_on_list
> /of_pwrseq_off_list instead (eg, USB hub driver).
>
> Signed-off-by: Peter Chen <peter.chen@nxp.com>
> Tested-by Joshua Clayton <stillcompiling@gmail.com>
> Reviewed-by: Matthias Kaehlcke <mka@chromium.org>
> Tested-by: Matthias Kaehlcke <mka@chromium.org>

first of all, glad to see this move forward. I've only some qualms with the 
static number of allocated power sequences below.

[...]

> diff --git a/drivers/power/pwrseq/Kconfig b/drivers/power/pwrseq/Kconfig
> new file mode 100644
> index 0000000..dff5e35
> --- /dev/null
> +++ b/drivers/power/pwrseq/Kconfig
> @@ -0,0 +1,45 @@
> +#
> +# 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.
> +
> +config PWRSEQ_GENERIC_INSTANCE_NUMBER
> +	int "Number of Generic Power Sequence Instance"
> +	depends on PWRSEQ_GENERIC
> +	range 1 10
> +	default 2
> +	help
> +	   Usually, there are not so many devices needs power sequence, we set two
> +	   as default value.

limiting this to some arbitary compile-time number somehow seems crippling for 
the single-image approach. I.e. a distribution might select something and 
during its lifetime the board requiring n+1 power-sequences appears and thus 
needs a different kernel version just to support that additional sequence.

Also, board designers are creative, and there were already complex examples 
mentioned elsewhere, so nothing keeps people from inventing something even 
more complex.

[...]

> diff --git a/drivers/power/pwrseq/pwrseq_generic.c
> b/drivers/power/pwrseq/pwrseq_generic.c new file mode 100644
> index 0000000..bcd16c3
> --- /dev/null
> +++ b/drivers/power/pwrseq/pwrseq_generic.c

[...]

> +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 != -ENOENT)
> +				goto err_put_clks;
> +			pwrseq_gen->clks[clk] = NULL;
> +			break;
> +		}
> +	}
> +
> +	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);
> +		of_property_read_u32(np, "reset-duration-us",
> +				&pwrseq_gen->duration_us);
> +	} else {
> +		if (reset_gpio == -ENOENT)
> +			return 0;
> +
> +		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;
> +}
> +
> +static const struct of_device_id generic_id_table[] = {
> +	{ .compatible = "generic",},
> +	{ /* sentinel */ }
> +};
> +
> +static int __init pwrseq_generic_register(void)
> +{
> +	struct pwrseq_generic *pwrseq_gen;
> +	int i;
> +
> +	for (i = 0; i < CONFIG_PWRSEQ_GENERIC_INSTANCE_NUMBER; i++) {
> +		pwrseq_gen = kzalloc(sizeof(*pwrseq_gen), GFP_KERNEL);
> +		if (!pwrseq_gen)
> +			return -ENOMEM;
> +
> +		pwrseq_gen->pwrseq.pwrseq_of_match_table = generic_id_table;
> +		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;
> +
> +		pwrseq_register(&pwrseq_gen->pwrseq);
> +	}
> +
> +	return 0;
> +}
> +postcore_initcall(pwrseq_generic_register)

I see that you need to have it preallocated for the compatible matching, but 
wouldn't it also work to either just register the type and allocate 
dynamically or otherwise just allocate a new spare everytime 
pwrseq_generic_get() picks up the previous spare?

That way the total number of power sequences can still be dynamic without 
haggling over how many power sequences should be the build-default in the 
generic configs.

  reply	other threads:[~2016-10-12 10:32 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-09-20  3:36 [PATCH v7 0/8] power: add power sequence library Peter Chen
2016-09-20  3:36 ` [PATCH v7 1/8] binding-doc: power: pwrseq-generic: add binding doc for generic " Peter Chen
2016-09-20  3:36 ` [PATCH v7 2/8] power: add " Peter Chen
2016-10-12 10:30   ` Heiko Stuebner [this message]
2016-10-13  1:22     ` Peter Chen
2016-10-13  7:04       ` Heiko Stuebner
2016-10-13  8:57         ` Peter Chen
2016-09-20  3:36 ` [PATCH v7 3/8] binding-doc: usb: usb-device: add optional properties for power sequence Peter Chen
2016-09-20  3:36 ` [PATCH v7 4/8] usb: core: add power sequence handling for USB devices Peter Chen
2016-09-20  3:36 ` [PATCH v7 5/8] usb: chipidea: let chipidea core device of_node equal's glue layer device of_node Peter Chen
2016-09-20  3:36 ` [PATCH v7 6/8] ARM: dts: imx6qdl: Enable usb node children with <reg> Peter Chen
2016-09-20  3:36 ` [PATCH v7 7/8] ARM: dts: imx6qdl-udoo.dtsi: fix onboard USB HUB property Peter Chen
2016-09-20  3:36 ` [PATCH v7 8/8] ARM: dts: imx6q-evi: Fix onboard hub reset line Peter Chen
2016-09-26  6:26 ` [PATCH v7 0/8] power: add power sequence library Peter Chen
2016-10-08  8:56   ` Peter Chen
2016-09-27 23:30 ` Maciej S. Szmigiero
2016-09-27 23:53   ` Peter Chen

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4691591.qnCGxrEkrZ@phil \
    --to=heiko@sntech.de \
    --cc=arnd@arndb.de \
    --cc=broonie@kernel.org \
    --cc=dbaryshkov@gmail.com \
    --cc=devicetree@vger.kernel.org \
    --cc=dwmw3@infradead.org \
    --cc=festevam@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=k.kozlowski@samsung.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=mail@maciej.szmigiero.name \
    --cc=mark.rutland@arm.com \
    --cc=mka@chromium.org \
    --cc=oscar@naiandei.net \
    --cc=p.zabel@pengutronix.de \
    --cc=pawel.moll@arm.com \
    --cc=peter.chen@nxp.com \
    --cc=robh+dt@kernel.org \
    --cc=s.hauer@pengutronix.de \
    --cc=shawnguo@kernel.org \
    --cc=sre@kernel.org \
    --cc=stephen.boyd@linaro.org \
    --cc=stern@rowland.harvard.edu \
    --cc=stillcompiling@gmail.com \
    --cc=troy.kisky@boundarydevices.com \
    --cc=ulf.hansson@linaro.org \
    --cc=vaibhav.hiremath@linaro.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).