All of lore.kernel.org
 help / color / mirror / Atom feed
From: Jon Hunter <jonathanh@nvidia.com>
To: Mikko Perttunen <mperttunen@nvidia.com>,
	thierry.reding@gmail.com, tbergstrom@nvidia.com
Cc: gnurou@gmail.com, Andrew Chew <achew@nvidia.com>,
	swarren@wwwdotorg.org, dri-devel@lists.freedesktop.org,
	linux-tegra@vger.kernel.org, amerilainen@nvidia.com,
	linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v2 2/4] drm/tegra: Add VIC support
Date: Mon, 20 Jul 2015 10:56:00 +0100	[thread overview]
Message-ID: <55ACC5B0.20703@nvidia.com> (raw)
In-Reply-To: <55ACB6A2.9030906@nvidia.com>


On 20/07/15 09:51, Mikko Perttunen wrote:
> On 07/20/2015 11:28 AM, Jon Hunter wrote:
>> Hi Mikko,
> 
> Hi!
> 
>> ...
>>> +static int vic_runtime_resume(struct device *dev)
>>> +{
>>> +	struct vic *vic = dev_get_drvdata(dev);
>>> +	int err;
>>> +
>>> +	err = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_VIC,
>>> +						vic->clk, vic->rst);
>>
>> I would like to deprecate use of the above function [1]. I have been
>> trying to migrate driver to use a new API instead of the above.
> 
> Yes, we will need to coordinate the API change here. I kept the old way
> here to not depend on your series for now.
> 
>>
>>> +	if (err < 0)
>>> +		dev_err(dev, "failed to power up device\n");
>>> +
>>> +	return err;
>>> +}
>>> +
>>> ...
>>> +
>>> +	pm_runtime_enable(&pdev->dev);
>>> +	if (!pm_runtime_enabled(&pdev->dev)) {
>>> +		err = vic_runtime_resume(&pdev->dev);
>>> +		if (err < 0)
>>> +			goto unregister_client;
>>> +	}
>>
>> I don't see why pm_runtime_enable() should ever fail to enable
>> pm_runtime here. Hence, the if-statement appears to be redundant. If you
>> are trying to determine whether rpm is supported for the power-domains
>> then you should simply check to see if the DT node for the device has
>> the "power-domains" property. See my series [1].
> 
> The intention is that if runtime PM is not enabled, the power domain is
> still brought up. On a cursory look, it seems like with your patch, this
> should indeed work fine without this check if CONFIG_PM is enabled. Now,
> that might always be enabled? If so, then everything would be fine; if
> this lands after your series, we could even just make the power-domains
> prop mandatory and not implement the legacy mode at all. If not, then
> AFAIU we must still keep this path.

Ok, I see you just wish to test it is enabled in the kernel config. Then
yes that would make sense and the above is fine.

Cheers
Jon
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

WARNING: multiple messages have this Message-ID (diff)
From: jonathanh@nvidia.com (Jon Hunter)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v2 2/4] drm/tegra: Add VIC support
Date: Mon, 20 Jul 2015 10:56:00 +0100	[thread overview]
Message-ID: <55ACC5B0.20703@nvidia.com> (raw)
In-Reply-To: <55ACB6A2.9030906@nvidia.com>


On 20/07/15 09:51, Mikko Perttunen wrote:
> On 07/20/2015 11:28 AM, Jon Hunter wrote:
>> Hi Mikko,
> 
> Hi!
> 
>> ...
>>> +static int vic_runtime_resume(struct device *dev)
>>> +{
>>> +	struct vic *vic = dev_get_drvdata(dev);
>>> +	int err;
>>> +
>>> +	err = tegra_powergate_sequence_power_up(TEGRA_POWERGATE_VIC,
>>> +						vic->clk, vic->rst);
>>
>> I would like to deprecate use of the above function [1]. I have been
>> trying to migrate driver to use a new API instead of the above.
> 
> Yes, we will need to coordinate the API change here. I kept the old way
> here to not depend on your series for now.
> 
>>
>>> +	if (err < 0)
>>> +		dev_err(dev, "failed to power up device\n");
>>> +
>>> +	return err;
>>> +}
>>> +
>>> ...
>>> +
>>> +	pm_runtime_enable(&pdev->dev);
>>> +	if (!pm_runtime_enabled(&pdev->dev)) {
>>> +		err = vic_runtime_resume(&pdev->dev);
>>> +		if (err < 0)
>>> +			goto unregister_client;
>>> +	}
>>
>> I don't see why pm_runtime_enable() should ever fail to enable
>> pm_runtime here. Hence, the if-statement appears to be redundant. If you
>> are trying to determine whether rpm is supported for the power-domains
>> then you should simply check to see if the DT node for the device has
>> the "power-domains" property. See my series [1].
> 
> The intention is that if runtime PM is not enabled, the power domain is
> still brought up. On a cursory look, it seems like with your patch, this
> should indeed work fine without this check if CONFIG_PM is enabled. Now,
> that might always be enabled? If so, then everything would be fine; if
> this lands after your series, we could even just make the power-domains
> prop mandatory and not implement the legacy mode at all. If not, then
> AFAIU we must still keep this path.

Ok, I see you just wish to test it is enabled in the kernel config. Then
yes that would make sense and the above is fine.

Cheers
Jon

  reply	other threads:[~2015-07-20  9:56 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-07-20  7:54 [PATCH v2 0/4] Add VIC support for Tegra124 Mikko Perttunen
2015-07-20  7:54 ` Mikko Perttunen
2015-07-20  7:54 ` Mikko Perttunen
     [not found] ` <1437378869-10451-1-git-send-email-mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2015-07-20  7:54   ` [PATCH v2 1/4] drm/tegra: Add falcon helper library Mikko Perttunen
2015-07-20  7:54     ` Mikko Perttunen
2015-07-20  7:54   ` [PATCH v2 2/4] drm/tegra: Add VIC support Mikko Perttunen
2015-07-20  7:54     ` Mikko Perttunen
2015-07-20  8:28     ` Jon Hunter
2015-07-20  8:28       ` Jon Hunter
     [not found]       ` <55ACB134.8010401-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2015-07-20  8:51         ` Mikko Perttunen
2015-07-20  8:51           ` Mikko Perttunen
2015-07-20  9:56           ` Jon Hunter [this message]
2015-07-20  9:56             ` Jon Hunter
     [not found]             ` <55ACC5B0.20703-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2015-07-20 10:17               ` Thierry Reding
2015-07-20 10:17                 ` Thierry Reding
2015-07-20  9:35       ` Thierry Reding
2015-07-20  9:35         ` Thierry Reding
2015-07-20  7:54   ` [PATCH v2 3/4] of: Add NVIDIA Tegra VIC binding Mikko Perttunen
2015-07-20  7:54     ` Mikko Perttunen
     [not found]     ` <1437378869-10451-4-git-send-email-mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
2015-07-20 17:20       ` Emil Velikov
2015-07-20 17:20         ` Emil Velikov
     [not found]         ` <CACvgo50X_mku2ha8Emi80j9ed+9Kn8SXpLk6ZDFxEKcjUAfCXw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2015-07-21  6:32           ` Mikko Perttunen
2015-07-21  6:32             ` Mikko Perttunen
2015-07-20  7:54   ` [PATCH v2 4/4] ARM: tegra: Add VIC for Tegra124 Mikko Perttunen
2015-07-20  7:54     ` Mikko Perttunen

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=55ACC5B0.20703@nvidia.com \
    --to=jonathanh@nvidia.com \
    --cc=achew@nvidia.com \
    --cc=amerilainen@nvidia.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=gnurou@gmail.com \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-tegra@vger.kernel.org \
    --cc=mperttunen@nvidia.com \
    --cc=swarren@wwwdotorg.org \
    --cc=tbergstrom@nvidia.com \
    --cc=thierry.reding@gmail.com \
    /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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.