Message-ID: <20160114143932.GE23082@ulmo>
Date:	Thu, 14 Jan 2016 15:39:32 +0100
From:	Thierry Reding <thierry.reding@...il.com>
To:	Jon Hunter <jonathanh@...dia.com>
Cc:	Philipp Zabel <p.zabel@...gutronix.de>,
	Stephen Warren <swarren@...dotorg.org>,
	Alexandre Courbot <gnurou@...il.com>,
	Rafael Wysocki <rjw@...ysocki.net>,
	Kevin Hilman <khilman@...nel.org>,
	Ulf Hansson <ulf.hansson@...aro.org>,
	Rob Herring <robh+dt@...nel.org>,
	Pawel Moll <pawel.moll@....com>,
	Mark Rutland <mark.rutland@....com>,
	Ian Campbell <ijc+devicetree@...lion.org.uk>,
	Kumar Gala <galak@...eaurora.org>,
	Vince Hsu <vinceh@...dia.com>, devicetree@...r.kernel.org,
	linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org,
	linux-pm@...r.kernel.org
Subject: Re: [PATCH V4 13/16] soc: tegra: pmc: Add generic PM domain support

On Fri, Dec 04, 2015 at 02:57:14PM +0000, Jon Hunter wrote:
[...]
> diff --git a/drivers/soc/tegra/pmc.c b/drivers/soc/tegra/pmc.c
[...]
> +static int tegra_powergate_power_down(struct tegra_powergate *pg,
> +				      bool enable_clocks)
> +{
> +	int err;
> +
> +	if (enable_clocks) {
> +		err = tegra_powergate_enable_clocks(pg);
> +		if (err)
> +			return err;
> +
> +		usleep_range(10, 20);
> +	}
> +
> +	err = tegra_powergate_reset_assert(pg);
> +	if (err)
> +		goto err_reset;
> +
> +	usleep_range(10, 20);
> +
> +	tegra_powergate_disable_clocks(pg);

There's no guarantee that all clocks are actually disabled at this
point. Will the power down and subsequent power up sequences still
work properly in such cases? If not perhaps we should add some way
of checking for that case here (WARN_ON?) to make sure we can fix
up all drivers to release their clock enable references.

> +static int tegra_powergate_of_get_clks(struct device *dev,
> +				       struct tegra_powergate *pg)
> +{
> +	struct clk *clk;
> +	unsigned int i, count;
> +	int err;
> +
> +	/*
> +	 * Determine number of clocks used by the powergate
> +	 */
> +	for (count = 0; ; count++) {
> +		clk = of_clk_get(pg->of_node, count);
> +		if (IS_ERR(clk))
> +			break;
> +
> +		clk_put(clk);
> +	}

of_count_phandle_with_args()?

> +static int tegra_powergate_of_get_resets(struct device *dev,
> +					 struct tegra_powergate *pg)
> +{
> +	struct reset_control *rst;
> +	unsigned int i, count;
> +	int err;
> +
> +	/*
> +	 * Determine number of resets used by the powergate
> +	 */
> +	for (count = 0; ; count++) {
> +		rst = of_reset_control_get_by_index(pg->of_node, count);
> +		if (IS_ERR(rst))
> +			break;
> +
> +		reset_control_put(rst);
> +	}

Same here.

> +static struct tegra_powergate *
> +tegra_powergate_add_one(struct tegra_pmc *pmc, struct device_node *np,
> +			struct generic_pm_domain *parent)
> +{
[...]
> +	dev_info(pmc->dev, "added power domain %s\n", pg->genpd.name);

That's a little chatty, isn't it? Perhaps dev_dbg()?

> +static int tegra_powergate_add(struct tegra_pmc *pmc, struct device_node *np,
> +			       struct generic_pm_domain *parent)
> +{
> +	struct tegra_powergate *pg;
> +	struct device_node *child;
> +	int err = 0;
> +
> +	for_each_child_of_node(np, child) {
> +		if (err)
> +			goto err;

This looks weird. Isn't the same check below good enough to catch all
cases?

> +
> +		pg = tegra_powergate_add_one(pmc, child, parent);
> +		if (IS_ERR(pg)) {
> +			err = PTR_ERR(pg);
> +			goto err;
> +		}
> +
> +		if (pg)
> +			err = tegra_powergate_add(pmc, child, pg->parent);
> +
> +err:
> +		of_node_put(child);
> +
> +		if (err)
> +			return err;

Perhaps break here instead of return?

> +	}
> +
> +	return err;
> +}
> +
> +static void tegra_powergate_remove(struct tegra_pmc *pmc)
> +{
> +	struct tegra_powergate *pg, *n;
> +
> +	list_for_each_entry_safe(pg, n, &pmc->powergates_list, node) {
> +		of_genpd_del_provider(pg->of_node);
> +		if (pg->parent)
> +			pm_genpd_remove_subdomain(pg->parent, &pg->genpd);
> +		pm_genpd_remove(&pg->genpd);
> +
> +		while (pg->num_clks--)
> +			clk_put(pg->clks[pg->num_clks]);
> +
> +		while (pg->num_resets--)
> +			reset_control_put(pg->resets[pg->num_resets]);
> +
> +		list_del(&pg->node);
> +	}
> +}

Are generic power domains reference counted? If not this will
potentially leave dangling pointers in user drivers, won't it?

That's a problem common to many subsystems, but maybe something to be
aware of.

> @@ -850,21 +1286,31 @@ static int tegra_pmc_probe(struct platform_device *pdev)
>  
>  	tegra_pmc_init_tsense_reset(pmc);
>  
> +	err = tegra_powergate_init(pmc);
> +	if (err < 0)
> +		return err;
> +
>  	if (IS_ENABLED(CONFIG_DEBUG_FS)) {
>  		err = tegra_powergate_debugfs_init();
>  		if (err < 0)
> -			return err;
> +			goto err_debugfs;
>  	}
>  
>  	err = register_restart_handler(&tegra_pmc_restart_handler);
>  	if (err) {
> -		debugfs_remove(pmc->debugfs);
>  		dev_err(&pdev->dev, "unable to register restart handler, %d\n",
>  			err);
> -		return err;
> +		goto err_restart;
>  	}
>  
>  	return 0;
> +
> +err_restart:
> +	debugfs_remove(pmc->debugfs);
> +err_debugfs:
> +	tegra_powergate_remove(pmc);

I prefer the labels to denote the action that is being taken rather than
the error that they respond to. remove_debugfs and remove_powergate
would therefore be better here, in my opinion. I think there were a
couple more in this and earlier patches.

Thierry

Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists