* Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences
From: Mark Brown @ 2012-08-01 13:55 UTC (permalink / raw)
To: Thierry Reding
Cc: Alex Courbot, Stephen Warren, Stephen Warren, Simon Glass,
Grant Likely, Rob Herring, Greg Kroah-Hartman, Arnd Bergmann,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
In-Reply-To: <20120801133814.GA19771-RM9K5IK7kjIyiCvfTdI0JKcOhU4Rzj621B7CTYaBSLdn68oJJulU0Q@public.gmane.org>
On Wed, Aug 01, 2012 at 03:38:14PM +0200, Thierry Reding wrote:
> On Wed, Aug 01, 2012 at 02:26:51PM +0100, Mark Brown wrote:
> > This is why __devinit data will only be discarded when this is not
> > possible.
> That's exactly my point. But I seem to have miserably failed to get that
> across. =)
We must be talking at cross purposes then. What I'm saying is that the
framework shouldn't rely on platform data and should assume that the
kernel might be configured so it can be discarded (by copying most
likely).
^ permalink raw reply
* Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences
From: Thierry Reding @ 2012-08-01 14:01 UTC (permalink / raw)
To: Mark Brown
Cc: Alex Courbot, Stephen Warren, Stephen Warren, Simon Glass,
Grant Likely, Rob Herring, Greg Kroah-Hartman, Arnd Bergmann,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
In-Reply-To: <20120801135531.GW11892-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 860 bytes --]
On Wed, Aug 01, 2012 at 02:55:31PM +0100, Mark Brown wrote:
> On Wed, Aug 01, 2012 at 03:38:14PM +0200, Thierry Reding wrote:
> > On Wed, Aug 01, 2012 at 02:26:51PM +0100, Mark Brown wrote:
>
> > > This is why __devinit data will only be discarded when this is not
> > > possible.
>
> > That's exactly my point. But I seem to have miserably failed to get that
> > across. =)
>
> We must be talking at cross purposes then. What I'm saying is that the
> framework shouldn't rely on platform data and should assume that the
> kernel might be configured so it can be discarded (by copying most
> likely).
Yes. I think this should be solved by the power_seq_build() function
which uses the platform data description of the sequence and builds it
into something for internal use that doesn't rely on anything in the
platform data.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 5/5] drivers/video/exynos/exynos_dp_core.c: use devm_ functions
From: Damien Cassou @ 2012-08-01 16:36 UTC (permalink / raw)
To: Jingoo Han
Cc: Sachin Kamat, kernel-janitors, Florian Tobias Schandinat,
linux-fbdev, linux-kernel
In-Reply-To: <004201cd6fa3$b0538b70$10faa250$%han@samsung.com>
On Wed, Aug 1, 2012 at 7:08 AM, Jingoo Han <jg1.han@samsung.com> wrote:
> To Damien,
> As Sachin Kamat mentioned, please change the subject more specific. For example,
>
> video: exynos_dp: use devm_clk_get function
ok, done in a new thread titled:
[PATCH v2] video: exynos_dp: use devm_clk_get function
Thank you for the feedback
--
Damien Cassou
http://damiencassou.seasidehosting.st
"Lambdas are relegated to relative obscurity until Java makes them
popular by not having them." James Iry
^ permalink raw reply
* Re: [PATCH v2] video: exynos_dp: use devm_clk_get function
From: Jingoo Han @ 2012-08-01 22:59 UTC (permalink / raw)
To: 'Damien Cassou'
Cc: 'Florian Tobias Schandinat', linux-fbdev, linux-kernel,
'Sachin Kamat', 'Jingoo Han'
In-Reply-To: <877gti1t48.fsf@gmail.com>
On Thursday, August 02, 2012 1:21 AM Damien Cassou wrote:
>
> From: Damien Cassou <damien.cassou@lifl.fr>
>
> The devm_clk_get function allocates memory that is released when a driver
> detaches. This patch uses this function for data that is allocated in the probe
> function of a platform device and is only freed in the remove function.
>
> Additionally, this patch removes a null check after platform_get_resource that
> is redundant with the one done by devm_request_and_ioremap.
>
> Signed-off-by: Damien Cassou <damien.cassou@lifl.fr>
Acked-by: Jingoo Han <jg1.han@samsung.com>
Thank you for sending the patch.
Best regards,
Jingoo Han
>
> ---
> Changed subject line and description
> drivers/video/exynos/exynos_dp_core.c | 27 +++++++--------------------
> 1 file changed, 7 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
> index c6c016a..00fe4f0 100644
> --- a/drivers/video/exynos/exynos_dp_core.c
> +++ b/drivers/video/exynos/exynos_dp_core.c
> @@ -872,7 +872,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
>
> dp->dev = &pdev->dev;
>
> - dp->clock = clk_get(&pdev->dev, "dp");
> + dp->clock = devm_clk_get(&pdev->dev, "dp");
> if (IS_ERR(dp->clock)) {
> dev_err(&pdev->dev, "failed to get clock\n");
> return PTR_ERR(dp->clock);
> @@ -881,31 +881,24 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> clk_enable(dp->clock);
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - if (!res) {
> - dev_err(&pdev->dev, "failed to get registers\n");
> - ret = -EINVAL;
> - goto err_clock;
> - }
>
> dp->reg_base = devm_request_and_ioremap(&pdev->dev, res);
> if (!dp->reg_base) {
> dev_err(&pdev->dev, "failed to ioremap\n");
> - ret = -ENOMEM;
> - goto err_clock;
> + return -ENOMEM;
> }
>
> dp->irq = platform_get_irq(pdev, 0);
> if (!dp->irq) {
> dev_err(&pdev->dev, "failed to get irq\n");
> - ret = -ENODEV;
> - goto err_clock;
> + return -ENODEV;
> }
>
> ret = devm_request_irq(&pdev->dev, dp->irq, exynos_dp_irq_handler, 0,
> "exynos-dp", dp);
> if (ret) {
> dev_err(&pdev->dev, "failed to request irq\n");
> - goto err_clock;
> + return ret;
> }
>
> dp->video_info = pdata->video_info;
> @@ -917,7 +910,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> ret = exynos_dp_detect_hpd(dp);
> if (ret) {
> dev_err(&pdev->dev, "unable to detect hpd\n");
> - goto err_clock;
> + return ret;
> }
>
> exynos_dp_handle_edid(dp);
> @@ -926,7 +919,7 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> dp->video_info->link_rate);
> if (ret) {
> dev_err(&pdev->dev, "unable to do link train\n");
> - goto err_clock;
> + return ret;
> }
>
> exynos_dp_enable_scramble(dp, 1);
> @@ -940,17 +933,12 @@ static int __devinit exynos_dp_probe(struct platform_device *pdev)
> ret = exynos_dp_config_video(dp, dp->video_info);
> if (ret) {
> dev_err(&pdev->dev, "unable to config video\n");
> - goto err_clock;
> + return ret;
> }
>
> platform_set_drvdata(pdev, dp);
>
> return 0;
> -
> -err_clock:
> - clk_put(dp->clock);
> -
> - return ret;
> }
>
> static int __devexit exynos_dp_remove(struct platform_device *pdev)
> @@ -962,7 +950,6 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev)
> pdata->phy_exit();
>
> clk_disable(dp->clock);
> - clk_put(dp->clock);
>
> return 0;
> }
^ permalink raw reply
* Re: [PATCH 1/4] video: Add support for the Solomon SSD1307 OLED Controller
From: Shawn Guo @ 2012-08-02 0:34 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <501904CB.9040104@free-electrons.com>
On Wed, Aug 01, 2012 at 12:28:27PM +0200, Maxime Ripard wrote:
> Thanks for the tip.
> However, I'm not sure this device will be mxs specific, since this
> controller is used on some other devices as well (I know for example
> that some uses it with the Arduino, even though it's not related to
> Linux), so relying on mxs-specific capabilities here might not be that
> great.
>
Ok, agreed. In that case, putting "1" in last cell of oled-reset-gpio
does not make any point then.
+Examples:
+ssd1307: oled@3c {
+ compatible = "solomon,ssd1307fb-i2c";
+ reg = <0x3c>;
+ pwms = <&pwm 4 3000>;
+ oled-reset-gpio = <&gpio2 7 1>;
+ oled-reset-active-low;
+};
--
Regards,
Shawn
^ permalink raw reply
* [PATCH] video: exynos-mipi-dsi: Add missing static storage class specifiers
From: Sachin Kamat @ 2012-08-02 6:26 UTC (permalink / raw)
To: linux-fbdev
Fixes the following sparse warnings:
drivers/video/exynos/exynos_mipi_dsi.c:208:22: warning:
symbol 'exynos_mipi_dsi_find_lcd_device' was not declared. Should it be static?
drivers/video/exynos/exynos_mipi_dsi.c:268:22: warning:
symbol 'exynos_mipi_dsi_bind_lcd_ddi' was not declared. Should it be static?
Signed-off-by: Sachin Kamat <sachin.kamat@linaro.org>
---
drivers/video/exynos/exynos_mipi_dsi.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/video/exynos/exynos_mipi_dsi.c b/drivers/video/exynos/exynos_mipi_dsi.c
index 4bc2b8a..ef68228 100644
--- a/drivers/video/exynos/exynos_mipi_dsi.c
+++ b/drivers/video/exynos/exynos_mipi_dsi.c
@@ -205,7 +205,8 @@ int exynos_mipi_dsi_register_lcd_device(struct mipi_dsim_lcd_device *lcd_dev)
return 0;
}
-struct mipi_dsim_ddi *exynos_mipi_dsi_find_lcd_device(struct mipi_dsim_lcd_driver *lcd_drv)
+static struct mipi_dsim_ddi *exynos_mipi_dsi_find_lcd_device(
+ struct mipi_dsim_lcd_driver *lcd_drv)
{
struct mipi_dsim_ddi *dsim_ddi, *next;
struct mipi_dsim_lcd_device *lcd_dev;
@@ -265,7 +266,8 @@ int exynos_mipi_dsi_register_lcd_driver(struct mipi_dsim_lcd_driver *lcd_drv)
}
-struct mipi_dsim_ddi *exynos_mipi_dsi_bind_lcd_ddi(struct mipi_dsim_device *dsim,
+static struct mipi_dsim_ddi *exynos_mipi_dsi_bind_lcd_ddi(
+ struct mipi_dsim_device *dsim,
const char *name)
{
struct mipi_dsim_ddi *dsim_ddi, *next;
--
1.7.4.1
^ permalink raw reply related
* [PATCH] video: s3c-fb: use devm_clk_get()
From: Jingoo Han @ 2012-08-02 6:59 UTC (permalink / raw)
To: linux-fbdev
The devm_ functions allocate memory that is released when a driver
detaches. This patch uses devm_clk_get() for these functions.
Signed-off-by: Jingoo Han <jg1.han@samsung.com>
---
drivers/video/s3c-fb.c | 24 +++++-------------------
1 files changed, 5 insertions(+), 19 deletions(-)
diff --git a/drivers/video/s3c-fb.c b/drivers/video/s3c-fb.c
index 69bf9d0..6c95126 100644
--- a/drivers/video/s3c-fb.c
+++ b/drivers/video/s3c-fb.c
@@ -1398,17 +1398,16 @@ static int __devinit s3c_fb_probe(struct platform_device *pdev)
spin_lock_init(&sfb->slock);
- sfb->bus_clk = clk_get(dev, "lcd");
+ sfb->bus_clk = devm_clk_get(dev, "lcd");
if (IS_ERR(sfb->bus_clk)) {
dev_err(dev, "failed to get bus clock\n");
- ret = PTR_ERR(sfb->bus_clk);
- goto err_sfb;
+ return PTR_ERR(sfb->bus_clk);
}
clk_enable(sfb->bus_clk);
if (!sfb->variant.has_clksel) {
- sfb->lcd_clk = clk_get(dev, "sclk_fimd");
+ sfb->lcd_clk = devm_clk_get(dev, "sclk_fimd");
if (IS_ERR(sfb->lcd_clk)) {
dev_err(dev, "failed to get lcd clock\n");
ret = PTR_ERR(sfb->lcd_clk);
@@ -1421,12 +1420,6 @@ static int __devinit s3c_fb_probe(struct platform_device *pdev)
pm_runtime_enable(sfb->dev);
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!res) {
- dev_err(dev, "failed to find registers\n");
- ret = -ENOENT;
- goto err_lcd_clk;
- }
-
sfb->regs = devm_request_and_ioremap(dev, res);
if (!sfb->regs) {
dev_err(dev, "failed to map registers\n");
@@ -1510,16 +1503,12 @@ err_pm_runtime:
err_lcd_clk:
pm_runtime_disable(sfb->dev);
- if (!sfb->variant.has_clksel) {
+ if (!sfb->variant.has_clksel)
clk_disable(sfb->lcd_clk);
- clk_put(sfb->lcd_clk);
- }
err_bus_clk:
clk_disable(sfb->bus_clk);
- clk_put(sfb->bus_clk);
-err_sfb:
return ret;
}
@@ -1541,13 +1530,10 @@ static int __devexit s3c_fb_remove(struct platform_device *pdev)
if (sfb->windows[win])
s3c_fb_release_win(sfb, sfb->windows[win]);
- if (!sfb->variant.has_clksel) {
+ if (!sfb->variant.has_clksel)
clk_disable(sfb->lcd_clk);
- clk_put(sfb->lcd_clk);
- }
clk_disable(sfb->bus_clk);
- clk_put(sfb->bus_clk);
pm_runtime_put_sync(sfb->dev);
pm_runtime_disable(sfb->dev);
--
1.7.1
^ permalink raw reply related
* Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences
From: Alex Courbot @ 2012-08-02 8:00 UTC (permalink / raw)
To: Stephen Warren
Cc: Stephen Warren, Thierry Reding, Simon Glass, Grant Likely,
Rob Herring, Greg Kroah-Hartman, Mark Brown, Arnd Bergmann,
linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-fbdev@vger.kernel.org, devicetree-discuss@lists.ozlabs.org
In-Reply-To: <50170EA0.1010408@wwwdotorg.org>
On 07/31/2012 07:45 AM, Stephen Warren wrote:
> Oh I see. That's a little confusing. Why not just reference the relevant
> resources directly in each step; something more like:
>
> gpio@1 {
> action = "enable-gpio";
> gpio = <&gpio 1 0>;
> };
>
> I guess that might make parsing/building a little harder, since you'd
> have to detect when you'd already done gpio_request() on a given GPIO
> and not repeat it or something like that, but to me this makes the DT a
> lot easier to comprehend.
I tried to move towards having the phandles directly in the sequences
themselves - that reminded me why I did not do that in the first place.
Let's say we have a sequence like this (reg property omitted on purpose):
power-on-sequence {
step@0 {
regulator = <&backlight_reg>;
enable;
};
step@1 {
delay = <10000>;
};
step@2 {
pwm = <&pwm 2 5000000>;
enable;
};
step@3 {
gpio = <&gpio 28 0>;
enable;
};
};
The problem is, how do we turn these phandles into the resource of
interest. The type of the resource can be infered by the name of the
property. The hard part is resolving the resource from the phandle - it
seems like the API just does not allow to do this. GPIO has
of_get_named_gpio, but AFAIK there are no equivalent for regulator
consumer and PWM: the only way to use the DT with them is through
get_regulator and get_pwm which work at the device level.
Or is there a way that I overlooked?
Thanks,
Alex.
^ permalink raw reply
* Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences
From: Thierry Reding @ 2012-08-02 8:21 UTC (permalink / raw)
To: Alex Courbot
Cc: Stephen Warren, Stephen Warren, Simon Glass, Grant Likely,
Rob Herring, Greg Kroah-Hartman, Mark Brown, Arnd Bergmann,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
In-Reply-To: <501A338D.7080105-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 1812 bytes --]
On Thu, Aug 02, 2012 at 05:00:13PM +0900, Alex Courbot wrote:
> On 07/31/2012 07:45 AM, Stephen Warren wrote:
> >Oh I see. That's a little confusing. Why not just reference the relevant
> >resources directly in each step; something more like:
> >
> > gpio@1 {
> > action = "enable-gpio";
> > gpio = <&gpio 1 0>;
> > };
> >
> >I guess that might make parsing/building a little harder, since you'd
> >have to detect when you'd already done gpio_request() on a given GPIO
> >and not repeat it or something like that, but to me this makes the DT a
> >lot easier to comprehend.
>
> I tried to move towards having the phandles directly in the
> sequences themselves - that reminded me why I did not do that in the
> first place. Let's say we have a sequence like this (reg property
> omitted on purpose):
>
> power-on-sequence {
> step@0 {
> regulator = <&backlight_reg>;
> enable;
> };
> step@1 {
> delay = <10000>;
> };
> step@2 {
> pwm = <&pwm 2 5000000>;
> enable;
> };
> step@3 {
> gpio = <&gpio 28 0>;
> enable;
> };
> };
>
> The problem is, how do we turn these phandles into the resource of
> interest. The type of the resource can be infered by the name of the
> property. The hard part is resolving the resource from the phandle -
> it seems like the API just does not allow to do this. GPIO has
> of_get_named_gpio, but AFAIK there are no equivalent for regulator
> consumer and PWM: the only way to use the DT with them is through
> get_regulator and get_pwm which work at the device level.
>
> Or is there a way that I overlooked?
No, you are right. Perhaps we should add exported functions that do the
equivalent of of_pwm_request() or the regulator_dev_lookup() and
of_get_regulator() pair.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences
From: Alex Courbot @ 2012-08-02 8:27 UTC (permalink / raw)
To: Thierry Reding
Cc: Stephen Warren, Stephen Warren, Simon Glass, Grant Likely,
Rob Herring, Greg Kroah-Hartman, Mark Brown, Arnd Bergmann,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
In-Reply-To: <20120802082157.GA14866-RM9K5IK7kjIyiCvfTdI0JKcOhU4Rzj621B7CTYaBSLdn68oJJulU0Q@public.gmane.org>
On Thu 02 Aug 2012 05:21:57 PM JST, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Thu, Aug 02, 2012 at 05:00:13PM +0900, Alex Courbot wrote:
>> On 07/31/2012 07:45 AM, Stephen Warren wrote:
>>> Oh I see. That's a little confusing. Why not just reference the relevant
>>> resources directly in each step; something more like:
>>>
>>> gpio@1 {
>>> action = "enable-gpio";
>>> gpio = <&gpio 1 0>;
>>> };
>>>
>>> I guess that might make parsing/building a little harder, since you'd
>>> have to detect when you'd already done gpio_request() on a given GPIO
>>> and not repeat it or something like that, but to me this makes the DT a
>>> lot easier to comprehend.
>>
>> I tried to move towards having the phandles directly in the
>> sequences themselves - that reminded me why I did not do that in the
>> first place. Let's say we have a sequence like this (reg property
>> omitted on purpose):
>>
>> power-on-sequence {
>> step@0 {
>> regulator = <&backlight_reg>;
>> enable;
>> };
>> step@1 {
>> delay = <10000>;
>> };
>> step@2 {
>> pwm = <&pwm 2 5000000>;
>> enable;
>> };
>> step@3 {
>> gpio = <&gpio 28 0>;
>> enable;
>> };
>> };
>>
>> The problem is, how do we turn these phandles into the resource of
>> interest. The type of the resource can be infered by the name of the
>> property. The hard part is resolving the resource from the phandle -
>> it seems like the API just does not allow to do this. GPIO has
>> of_get_named_gpio, but AFAIK there are no equivalent for regulator
>> consumer and PWM: the only way to use the DT with them is through
>> get_regulator and get_pwm which work at the device level.
>>
>> Or is there a way that I overlooked?
>
> No, you are right. Perhaps we should add exported functions that do the
> equivalent of of_pwm_request() or the regulator_dev_lookup() and
> of_get_regulator() pair.
How would that be looked with respect to "good DT practices"? I can
somehow understand the wish to restrain DT access to these functions
that integrate well with current workflows. Aren't we going to be
frowned upon if we make more low-level functions public?
Alex.
^ permalink raw reply
* Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences
From: Thierry Reding @ 2012-08-02 8:45 UTC (permalink / raw)
To: Alex Courbot
Cc: Stephen Warren, Stephen Warren, Simon Glass, Grant Likely,
Rob Herring, Greg Kroah-Hartman, Mark Brown, Arnd Bergmann,
linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-fbdev@vger.kernel.org, devicetree-discuss@lists.ozlabs.org
In-Reply-To: <501A3A00.7070207@nvidia.com>
[-- Attachment #1: Type: text/plain, Size: 2408 bytes --]
On Thu, Aug 02, 2012 at 05:27:44PM +0900, Alex Courbot wrote:
> On Thu 02 Aug 2012 05:21:57 PM JST, Thierry Reding wrote:
> >* PGP Signed by an unknown key
> >
> >On Thu, Aug 02, 2012 at 05:00:13PM +0900, Alex Courbot wrote:
> >>On 07/31/2012 07:45 AM, Stephen Warren wrote:
> >>>Oh I see. That's a little confusing. Why not just reference the relevant
> >>>resources directly in each step; something more like:
> >>>
> >>> gpio@1 {
> >>> action = "enable-gpio";
> >>> gpio = <&gpio 1 0>;
> >>> };
> >>>
> >>>I guess that might make parsing/building a little harder, since you'd
> >>>have to detect when you'd already done gpio_request() on a given GPIO
> >>>and not repeat it or something like that, but to me this makes the DT a
> >>>lot easier to comprehend.
> >>
> >>I tried to move towards having the phandles directly in the
> >>sequences themselves - that reminded me why I did not do that in the
> >>first place. Let's say we have a sequence like this (reg property
> >>omitted on purpose):
> >>
> >> power-on-sequence {
> >> step@0 {
> >> regulator = <&backlight_reg>;
> >> enable;
> >> };
> >> step@1 {
> >> delay = <10000>;
> >> };
> >> step@2 {
> >> pwm = <&pwm 2 5000000>;
> >> enable;
> >> };
> >> step@3 {
> >> gpio = <&gpio 28 0>;
> >> enable;
> >> };
> >> };
> >>
> >>The problem is, how do we turn these phandles into the resource of
> >>interest. The type of the resource can be infered by the name of the
> >>property. The hard part is resolving the resource from the phandle -
> >>it seems like the API just does not allow to do this. GPIO has
> >>of_get_named_gpio, but AFAIK there are no equivalent for regulator
> >>consumer and PWM: the only way to use the DT with them is through
> >>get_regulator and get_pwm which work at the device level.
> >>
> >>Or is there a way that I overlooked?
> >
> >No, you are right. Perhaps we should add exported functions that do the
> >equivalent of of_pwm_request() or the regulator_dev_lookup() and
> >of_get_regulator() pair.
>
> How would that be looked with respect to "good DT practices"? I can
> somehow understand the wish to restrain DT access to these functions
> that integrate well with current workflows. Aren't we going to be
> frowned upon if we make more low-level functions public?
Yes, I think that's exactly what will happen.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences
From: Alex Courbot @ 2012-08-02 9:20 UTC (permalink / raw)
To: Thierry Reding
Cc: Stephen Warren, Stephen Warren, Simon Glass, Grant Likely,
Rob Herring, Greg Kroah-Hartman, Mark Brown, Arnd Bergmann,
linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-fbdev@vger.kernel.org, devicetree-discuss@lists.ozlabs.org
In-Reply-To: <20120802084541.GA15042@avionic-0098.adnet.avionic-design.de>
On Thu 02 Aug 2012 05:45:41 PM JST, Thierry Reding wrote:
> * PGP Signed by an unknown key
>
> On Thu, Aug 02, 2012 at 05:27:44PM +0900, Alex Courbot wrote:
>> On Thu 02 Aug 2012 05:21:57 PM JST, Thierry Reding wrote:
>>>> Old Signed by an unknown key
>>>
>>> On Thu, Aug 02, 2012 at 05:00:13PM +0900, Alex Courbot wrote:
>>>> On 07/31/2012 07:45 AM, Stephen Warren wrote:
>>>>> Oh I see. That's a little confusing. Why not just reference the relevant
>>>>> resources directly in each step; something more like:
>>>>>
>>>>> gpio@1 {
>>>>> action = "enable-gpio";
>>>>> gpio = <&gpio 1 0>;
>>>>> };
>>>>>
>>>>> I guess that might make parsing/building a little harder, since you'd
>>>>> have to detect when you'd already done gpio_request() on a given GPIO
>>>>> and not repeat it or something like that, but to me this makes the DT a
>>>>> lot easier to comprehend.
>>>>
>>>> I tried to move towards having the phandles directly in the
>>>> sequences themselves - that reminded me why I did not do that in the
>>>> first place. Let's say we have a sequence like this (reg property
>>>> omitted on purpose):
>>>>
>>>> power-on-sequence {
>>>> step@0 {
>>>> regulator = <&backlight_reg>;
>>>> enable;
>>>> };
>>>> step@1 {
>>>> delay = <10000>;
>>>> };
>>>> step@2 {
>>>> pwm = <&pwm 2 5000000>;
>>>> enable;
>>>> };
>>>> step@3 {
>>>> gpio = <&gpio 28 0>;
>>>> enable;
>>>> };
>>>> };
>>>>
>>>> The problem is, how do we turn these phandles into the resource of
>>>> interest. The type of the resource can be infered by the name of the
>>>> property. The hard part is resolving the resource from the phandle -
>>>> it seems like the API just does not allow to do this. GPIO has
>>>> of_get_named_gpio, but AFAIK there are no equivalent for regulator
>>>> consumer and PWM: the only way to use the DT with them is through
>>>> get_regulator and get_pwm which work at the device level.
>>>>
>>>> Or is there a way that I overlooked?
>>>
>>> No, you are right. Perhaps we should add exported functions that do the
>>> equivalent of of_pwm_request() or the regulator_dev_lookup() and
>>> of_get_regulator() pair.
>>
>> How would that be looked with respect to "good DT practices"? I can
>> somehow understand the wish to restrain DT access to these functions
>> that integrate well with current workflows. Aren't we going to be
>> frowned upon if we make more low-level functions public?
>
> Yes, I think that's exactly what will happen.
Probably not worth to try this then?
Alex.
^ permalink raw reply
* Re: [RFC PATCH 22/29] OMAPDSS: DISPC: Setup writeback go, enable and channel_in functions
From: Mahapatra, Chandrabhanu @ 2012-08-02 10:07 UTC (permalink / raw)
To: Archit Taneja; +Cc: tomi.valkeinen, rohitkc, linux-omap, linux-fbdev
In-Reply-To: <1324989432-3625-23-git-send-email-archit@ti.com>
On Tue, Dec 27, 2011 at 6:07 PM, Archit Taneja <archit@ti.com> wrote:
> Fill up dispc_wb_go(), dispc_wb_go_busy(), dispc_wb_enable() and
> dispc_wb_get_channel_in()/set_channel_in() with writeback register writes. Make
> a minor modification in dss_wb_write_regs_extra() to pass the plane_id instead
> of the writeback id when calling dispc_wb_set_channel_in().
>
> Setup dispc_wb_enable() as dispc_enable_lcd_out() function and wait for the
> FRAMEDONEWB interrupt while disabling writeback.
>
> Signed-off-by: Archit Taneja <archit@ti.com>
> ---
> drivers/video/omap2/dss/apply.c | 2 +-
> drivers/video/omap2/dss/dispc.c | 68 +++++++++++++++++++++++++++++++++++----
> drivers/video/omap2/dss/dss.h | 2 +-
> 3 files changed, 63 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
> index dd1fd419..57b061f 100644
> --- a/drivers/video/omap2/dss/apply.c
> +++ b/drivers/video/omap2/dss/apply.c
> @@ -690,7 +690,7 @@ static void dss_wb_write_regs_extra(struct omap_dss_writeback *wb)
> if (!wp->extra_info_dirty)
> return;
>
> - dispc_wb_set_channel_in(wb->id, wp->channel_in);
> + dispc_wb_set_channel_in(wb->plane_id, wp->channel_in);
> dispc_ovl_set_fifo_threshold(wb->plane_id, wp->fifo_low, wp->fifo_high);
>
> wp->extra_info_dirty = false;
> diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
> index c7de56d..cbce120 100644
> --- a/drivers/video/omap2/dss/dispc.c
> +++ b/drivers/video/omap2/dss/dispc.c
> @@ -510,12 +510,26 @@ void dispc_mgr_go(enum omap_channel channel)
>
> bool dispc_wb_go_busy(int id)
> {
> - return false;
> + return REG_GET(DISPC_CONTROL2, 6, 6) = 1;
> }
>
> void dispc_wb_go(int id)
> {
> - return;
> + struct omap_dss_writeback *wb = omap_dss_get_writeback(id);
> + bool enable, go;
> +
> + enable = REG_GET(DISPC_OVL_ATTRIBUTES(wb->plane_id), 0, 0) = 1;
> +
> + if (!enable)
> + return;
> +
> + go = REG_GET(DISPC_CONTROL2, 6, 6) = 1;
> + if (go) {
> + DSSERR("GO bit not down for WB\n");
> + return;
> + }
> +
> + REG_FLD_MOD(DISPC_CONTROL2, 1, 6, 6);
> }
>
> static void dispc_ovl_write_firh_reg(enum omap_plane plane, int reg, u32 value)
> @@ -903,16 +917,24 @@ static enum omap_channel dispc_ovl_get_channel_out(enum omap_plane plane)
> return channel;
> }
>
> -void dispc_wb_set_channel_in(int id, enum dss_writeback_channel_in ch_in)
> +void dispc_wb_set_channel_in(int plane, enum dss_writeback_channel_in ch_in)
> {
> - return;
> + REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), ch_in, 18, 16);
> }
>
> static enum omap_channel dispc_wb_get_channel_in(int plane)
> {
> - /* Return LCD channel for now */
> + int channel_in = REG_GET(DISPC_OVL_ATTRIBUTES(plane), 18, 16);
>
> - return OMAP_DSS_CHANNEL_LCD;
> + switch (channel_in) {
> + case OMAP_DSS_WB_LCD2_MGR:
> + return OMAP_DSS_CHANNEL_LCD2;
> + case OMAP_DSS_WB_TV_MGR:
> + return OMAP_DSS_CHANNEL_DIGIT;
> + case OMAP_DSS_WB_LCD1_MGR:
> + default:
> + return OMAP_DSS_CHANNEL_LCD;
> + };
> }
>
What about other cases of pipelines and overlays here? You have
considered only overlays and that too ignored OMAP_DSS_CHANNEL_LCD3.
The channel_in parameter of DISPC_WB_ATTRIBUTES does handle the
following cases as well:
0x0: Primary LCD overlay output
0x1: Secondary LCD output
0x2: TV overlay output
0x7: Third LCD output
0x3: Graphics pipeline output
0x4: Video1 pipeline output
0x5: Video2 pipeline output
0x6: Video3 pipeline output
> static void dispc_ovl_set_burst_size(enum omap_plane plane,
> @@ -2138,7 +2160,39 @@ void dispc_mgr_enable(enum omap_channel channel, bool enable)
>
> void dispc_wb_enable(int id, bool enable)
> {
> - return;
> + struct omap_dss_writeback *wb = omap_dss_get_writeback(id);
> + enum omap_plane plane = wb->plane_id;
> + struct completion frame_done_completion;
> + bool is_on;
> + int r;
> + u32 irq;
> +
> + is_on = REG_GET(DISPC_OVL_ATTRIBUTES(plane), 0, 0);
> + irq = DISPC_IRQ_FRAMEDONEWB;
> +
> + if (!enable && is_on) {
> + init_completion(&frame_done_completion);
> +
> + r = omap_dispc_register_isr(dispc_disable_isr,
> + &frame_done_completion, irq);
> +
> + if (r)
> + DSSERR("failed to register FRAMEDONEWB isr\n");
> + }
> +
> + REG_FLD_MOD(DISPC_OVL_ATTRIBUTES(plane), enable ? 1 : 0, 0, 0);
> +
> + if (!enable && is_on) {
> + if (!wait_for_completion_timeout(&frame_done_completion,
> + msecs_to_jiffies(100)))
> + DSSERR("timeout waiting for FRAMEDONEWB\n");
> +
> + r = omap_dispc_unregister_isr(dispc_disable_isr,
> + &frame_done_completion, irq);
> +
> + if (r)
> + DSSERR("failed to unregister FRAMEDONEWB isr\n");
> + }
> }
>
> void dispc_lcd_enable_signal_polarity(bool act_high)
> diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
> index 69b4793..c05658b 100644
> --- a/drivers/video/omap2/dss/dss.h
> +++ b/drivers/video/omap2/dss/dss.h
> @@ -498,7 +498,7 @@ void dispc_wb_go(int id);
> int dispc_wb_setup(int id, struct omap_dss_writeback_info *wi,
> u16 in_width, u16 in_height);
> void dispc_wb_enable(int id, bool enable);
> -void dispc_wb_set_channel_in(int id, enum dss_writeback_channel_in ch_in);
> +void dispc_wb_set_channel_in(int plane, enum dss_writeback_channel_in ch_in);
>
> /* VENC */
> #ifdef CONFIG_OMAP2_DSS_VENC
> --
> 1.7.4.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Chandrabhanu Mahapatra
Texas Instruments India Pvt. Ltd.
^ permalink raw reply
* Re: [RFC PATCH 21/29] OMAPDSS: DISPC: Configure overlay-like parameters in dispc_wb_setup
From: Chandrabhanu Mahapatra @ 2012-08-02 10:22 UTC (permalink / raw)
To: Archit Taneja; +Cc: tomi.valkeinen, rohitkc, linux-omap, linux-fbdev
In-Reply-To: <1324989432-3625-22-git-send-email-archit@ti.com>
On Tuesday 27 December 2011 06:07 PM, Archit Taneja wrote:
> Call dispc_plane_setup() through dispc_wb_setup() to configure overlay-like
> parameters. Create a helper function in writeback.c called dss_wb_calc_params()
> which for now calculates the input width and height which goes to writeback.
> Create a dummy dispc function which returns the channel of the manager to which
> the writeback pipeline is connected.
>
> The parameters in dispc_plane_setup() which do not hold for writeback are filled
> passed as zeroes or false, dispc_plane_setup() takes care of not configuring
> them if the plane is writeback.
>
> Signed-off-by: Archit Taneja <archit@ti.com>
> ---
> drivers/video/omap2/dss/apply.c | 5 ++++-
> drivers/video/omap2/dss/dispc.c | 33 +++++++++++++++++++++++++++++++--
> drivers/video/omap2/dss/dss.h | 6 +++++-
> drivers/video/omap2/dss/writeback.c | 17 +++++++++++++++++
> 4 files changed, 57 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/video/omap2/dss/apply.c b/drivers/video/omap2/dss/apply.c
> index a17cc47..dd1fd419 100644
> --- a/drivers/video/omap2/dss/apply.c
> +++ b/drivers/video/omap2/dss/apply.c
> @@ -661,6 +661,7 @@ static void dss_wb_write_regs(struct omap_dss_writeback *wb)
> {
> struct wb_priv_data *wp = get_wb_priv(wb);
> struct omap_dss_writeback_info *wi;
> + u16 in_width, in_height;
> int r;
>
> if (!wp->enabled || !wp->info_dirty)
> @@ -670,7 +671,9 @@ static void dss_wb_write_regs(struct omap_dss_writeback *wb)
>
> wi = &wp->info;
>
> - r = dispc_wb_setup(wb->id, wi);
> + dss_wb_calc_params(wb, wi, &in_width, &in_height);
> +
> + r = dispc_wb_setup(wb->id, wi, in_width, in_height);
> if (r) {
> DSSERR("dispc_wb_setup failed\n");
> return;
> diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
> index 3a40f8e..c7de56d 100644
> --- a/drivers/video/omap2/dss/dispc.c
> +++ b/drivers/video/omap2/dss/dispc.c
> @@ -908,6 +908,13 @@ void dispc_wb_set_channel_in(int id, enum dss_writeback_channel_in ch_in)
> return;
> }
>
> +static enum omap_channel dispc_wb_get_channel_in(int plane)
> +{
> + /* Return LCD channel for now */
> +
> + return OMAP_DSS_CHANNEL_LCD;
> +}
> +
> static void dispc_ovl_set_burst_size(enum omap_plane plane,
> enum omap_burst_size burst_size)
> {
> @@ -1935,9 +1942,31 @@ int dispc_ovl_setup(enum omap_plane plane, struct omap_overlay_info *oi,
> return r;
> }
>
> -int dispc_wb_setup(int id, struct omap_dss_writeback_info *wi)
> +int dispc_wb_setup(int id, struct omap_dss_writeback_info *wi,
> + u16 in_width, u16 in_height)
> {
> - return 0;
> + int r;
> + struct omap_dss_writeback *wb = omap_dss_get_writeback(id);
> + const int pos_x = 0, pos_y = 0;
> + const u8 zorder = 0, global_alpha = 0;
> + const bool chroma_upscale = false, ilace = false, replication = false;
> + enum omap_channel channel;
> +
> + channel = dispc_wb_get_channel_in(wb->plane_id);
> +
> + DSSDBG("dispc_wb_setup %d, pa %x, pa_uv %x, %d,%d -> %dx%d, cmode %x, "
> + "rot %d, mir %d, chan %d\n",
> + wb->id, wi->paddr, wi->p_uv_addr, in_width, in_height,
> + wi->buf_width, wi->buf_height, wi->color_mode, wi->rotation,
> + wi->mirror, channel);
> +
> + r = dispc_plane_setup(wb->plane_id, channel, wb->caps, wi->paddr,
> + wi->p_uv_addr, in_width, pos_x, pos_y, in_width, in_height,
> + wi->buf_width, wi->buf_height, wi->color_mode, wi->rotation,
> + wi->mirror, zorder, wi->pre_mult_alpha, global_alpha,
> + wi->rotation_type, chroma_upscale, ilace, replication);
The only note worthy difference here is use of omap_dss_writeback_info
*wi instead of omap_overlay_info *oi in dispc_ovl_setup(). If
omap_overlay_info can be used instead of omap_dss_writeback_info then
the same dispc_ovl_setup() would have been used to handle writeback as a
plane just like others but with extra checks for (plane = OMAP_DSS_WB).
I think this way it would have been much cleaner otherwise it looks good.
> +
> + return r;
> }
>
> int dispc_ovl_enable(enum omap_plane plane, bool enable)
> diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
> index 1b128f1..69b4793 100644
> --- a/drivers/video/omap2/dss/dss.h
> +++ b/drivers/video/omap2/dss/dss.h
> @@ -249,6 +249,9 @@ void dss_init_writeback(void);
> void dss_uninit_writeback(void);
> int writeback_init_display(struct omap_dss_device *dssdev);
> enum dss_writeback_channel_in dss_wb_calc_channel_in(struct omap_dss_writeback *wb);
> +void dss_wb_calc_params(struct omap_dss_writeback *wb,
> + struct omap_dss_writeback_info *wi, u16 *in_width,
> + u16 *in_height);
> int dss_wb_simple_check(struct omap_dss_writeback *wb,
> const struct omap_dss_writeback_info *info);
>
> @@ -492,7 +495,8 @@ void dispc_mgr_setup(enum omap_channel channel,
>
> bool dispc_wb_go_busy(int id);
> void dispc_wb_go(int id);
> -int dispc_wb_setup(int id, struct omap_dss_writeback_info *wi);
> +int dispc_wb_setup(int id, struct omap_dss_writeback_info *wi,
> + u16 in_width, u16 in_height);
> void dispc_wb_enable(int id, bool enable);
> void dispc_wb_set_channel_in(int id, enum dss_writeback_channel_in ch_in);
>
> diff --git a/drivers/video/omap2/dss/writeback.c b/drivers/video/omap2/dss/writeback.c
> index 14103bf..7c4e9c0 100644
> --- a/drivers/video/omap2/dss/writeback.c
> +++ b/drivers/video/omap2/dss/writeback.c
> @@ -141,6 +141,23 @@ enum dss_writeback_channel_in dss_wb_calc_channel_in(struct omap_dss_writeback *
> }
> }
>
> +void dss_wb_calc_params(struct omap_dss_writeback *wb,
> + struct omap_dss_writeback_info *wi, u16 *in_width,
> + u16 *in_height)
> +{
> + struct omap_video_timings timings;
> + struct omap_dss_device *dssdev;
> + struct omap_overlay_manager *mgr;
> +
> + mgr = wb->dssdev->manager;
> + dssdev = mgr->get_display(mgr);
> +
> + dssdev->driver->get_timings(dssdev, &timings);
> +
> + *in_width = timings.x_res;
> + *in_height = timings.y_res;
> +}
> +
> int dss_wb_simple_check(struct omap_dss_writeback *wb,
> const struct omap_dss_writeback_info *info)
> {
--
Chandrabhanu Mahapatra
Texas Instruments India Pvt. Ltd.
^ permalink raw reply
* Re: [PATCH 1/4] video: Add support for the Solomon SSD1307 OLED Controller
From: Maxime Ripard @ 2012-08-02 13:35 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20120802003422.GA2253@S2101-09.ap.freescale.net>
Le 02/08/2012 02:34, Shawn Guo a écrit :
> On Wed, Aug 01, 2012 at 12:28:27PM +0200, Maxime Ripard wrote:
>> Thanks for the tip.
>> However, I'm not sure this device will be mxs specific, since this
>> controller is used on some other devices as well (I know for example
>> that some uses it with the Arduino, even though it's not related to
>> Linux), so relying on mxs-specific capabilities here might not be that
>> great.
>>
> Ok, agreed. In that case, putting "1" in last cell of oled-reset-gpio
> does not make any point then.
>
> +Examples:
> +ssd1307: oled@3c {
> + compatible = "solomon,ssd1307fb-i2c";
> + reg = <0x3c>;
> + pwms = <&pwm 4 3000>;
> + oled-reset-gpio = <&gpio2 7 1>;
> + oled-reset-active-low;
> +};
>
Ok, I will do that then.
Maxime
--
Maxime Ripard, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
^ permalink raw reply
* viafb on Clevo M5x0V laptop (+VT1631L)
From: Ondrej Zary @ 2012-08-02 18:03 UTC (permalink / raw)
To: Florian Tobias Schandinat; +Cc: linux-fbdev, Kernel development list
Hello,
I've got a Clevo M5x0V laptop which has VIA PM800 chipset (1106:3118) and
1280x768 LCD panel connected using VT1631L LVDS transmitter. There's a
schematic of this laptop available on the web which shows that VT1631L's I2C
port is not connected anywhere...
viafb does not work properly - LCD panel (and backlight) is turned off upon
module load and there's 640x480 signal on VGA connector (even when monitor
was not connected during module load).
Looking at the code, there's almost nothing for VT1631. What needs to be done
in order for the LCD to work? Xorg seems to work using openchrome (but it is
not able to restore the card on resume from S3):
(II) CHROME(0): Unable to get panel size from EDID. Return code: 0
(II) CHROME(0): ViaPanelGetNativeModeFromScratchPad
(II) CHROME(0): Native Panel Resolution is 1280x768
This is from kernel 3.5 with VIAFB_DEBUG and VIAFB_WARN enabled:
[ 153.051061] VIA Graphics Integration Chipset framebuffer 2.4 initializing
[ 153.051765] viafb 0000:01:00.0: power state changed by ACPI to D0
[ 153.051773] viafb 0000:01:00.0: power state changed by ACPI to D0
[ 153.053535] Device ID = 3259
[ 153.053553] FB Size = 4000
[ 153.059984] VIAFB PCI Probe!!
[ 153.060044] viafb: Probing I2C bus 0x26
[ 153.092761] viafb: Probing I2C bus 0x31
[ 155.588019] viafb: Probing I2C bus 0x2C
[ 155.619016] viafb: Finished I2C bus probing
[ 155.619021] parse_lcd_port: viafb_lcd_port:,interface:0
[ 155.619023] parse_dvi_port: viafb_dvi_port:,interface:0
[ 155.713224] TMDS Chip = 0
[ 155.713227] viafb_init_dvi_size()
[ 155.713229] viafb_dvi_sense!!
[ 155.714415] viafb_dvi_query_EDID!!
[ 155.716820] viafb_dvi_query_EDID!!
[ 155.719179] viafb_lvds_identify_vt1636.
[ 155.812018] viafb_lvds_identify_vt1636.
[ 155.814383] viafb_init_lcd_size()
[ 155.814384] fp_get_panel_id()
[ 155.814387] LVDS Chip = 0
[ 155.814389] LVDS1 output_interface = 6
[ 155.814391] LVDS2 output_interface = 6
[ 155.814405] viafb_check_var!
[ 155.815250] fbcon: Via (fb0) is primary device
[ 155.815257] viafb_open!
[ 155.818608] viafb_set_par!
[ 155.818720] via_set_primary_pitch(0x00000A00)
[ 155.818723] via_set_secondary_pitch(0x00000A00)
[ 155.818727] via_set_primary_color_depth(24)
[ 155.818730] via_set_secondary_color_depth(24)
[ 155.818788] viafb_pan_display, address = 0
[ 155.818789] via_set_primary_address(0x00000000)
[ 155.818793] via_set_secondary_address(0x00000000)
[ 155.819020] viafb_pan_display, address = 0
[ 155.819021] via_set_primary_address(0x00000000)
[ 155.819025] via_set_secondary_address(0x00000000)
[ 155.819044] viafb 2D engine: imageblit
...many imageblits...
[ 155.826516] Console: switching to colour frame buffer device 80x30
[ 155.826522] viafb_pan_display, address = 0
[ 155.826523] via_set_primary_address(0x00000000)
[ 155.826527] via_set_secondary_address(0x00000000)
...many imageblits...
[ 155.833889] fb0: Via frame buffer device 640x480-32bpp
--
Ondrej Zary
^ permalink raw reply
* Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences
From: Mark Brown @ 2012-08-02 18:11 UTC (permalink / raw)
To: Thierry Reding
Cc: Alex Courbot, Stephen Warren, Stephen Warren, Simon Glass,
Grant Likely, Rob Herring, Greg Kroah-Hartman, Arnd Bergmann,
linux-tegra@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-fbdev@vger.kernel.org, devicetree-discuss@lists.ozlabs.org
In-Reply-To: <20120802082157.GA14866@avionic-0098.adnet.avionic-design.de>
On Thu, Aug 02, 2012 at 10:21:57AM +0200, Thierry Reding wrote:
> On Thu, Aug 02, 2012 at 05:00:13PM +0900, Alex Courbot wrote:
> > The problem is, how do we turn these phandles into the resource of
> > interest. The type of the resource can be infered by the name of the
> > property. The hard part is resolving the resource from the phandle -
> > it seems like the API just does not allow to do this. GPIO has
> > of_get_named_gpio, but AFAIK there are no equivalent for regulator
> > consumer and PWM: the only way to use the DT with them is through
> > get_regulator and get_pwm which work at the device level.
> > Or is there a way that I overlooked?
> No, you are right. Perhaps we should add exported functions that do the
> equivalent of of_pwm_request() or the regulator_dev_lookup() and
> of_get_regulator() pair.
I missed some of the earlier bits of the thread here but why can't we do
device based lookups?
^ permalink raw reply
* Re: [PATCH v2] of: Add videomode helper
From: Stephen Warren @ 2012-08-02 19:35 UTC (permalink / raw)
To: Sascha Hauer
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org,
Laurent Pinchart, kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org
In-Reply-To: <1341388595-30672-1-git-send-email-s.hauer-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
On 07/04/2012 01:56 AM, Sascha Hauer wrote:
> This patch adds a helper function for parsing videomodes from the devicetree.
> The videomode can be either converted to a struct drm_display_mode or a
> struct fb_videomode.
> diff --git a/Documentation/devicetree/bindings/video/displaymode b/Documentation/devicetree/bindings/video/displaymode
> +Required properties:
> + - xres, yres: Display resolution
> + - left-margin, right-margin, hsync-len: Horizontal Display timing parameters
> + in pixels
> + upper-margin, lower-margin, vsync-len: Vertical display timing parameters in
> + lines
Perhaps bike-shedding, but...
For the margin property names, wouldn't it be better to use terminology
more commonly used for video timings rather than Linux FB naming. In
other words naming like:
hactive, hsync-len, hfront-porch, hback-porch?
> + - clock: displayclock in Hz
> +
> +Optional properties:
> + - width-mm, height-mm: Display dimensions in mm
> + - hsync-active-high (bool): Hsync pulse is active high
> + - vsync-active-high (bool): Vsync pulse is active high
> + - interlaced (bool): This is an interlaced mode
> + - doublescan (bool): This is a doublescan mode
> +
> +There are different ways of describing a display mode. The devicetree representation
> +corresponds to the one used by the Linux Framebuffer framework described here in
> +Documentation/fb/framebuffer.txt. This representation has been chosen because it's
> +the only format which does not allow for inconsistent parameters.Unlike the Framebuffer
> +framework the devicetree has the clock in Hz instead of ps.
As Rob mentioned, I think there shouldn't be any mention of Linux FB here.
> +
> +Example:
> +
> + display@0 {
This node appears to describe a video mode, not a display, hence the
node name seems wrong.
Many displays can support multiple different video modes. As mentioned
elsewhere, properties like display width/height are properties of the
display not the mode.
So, I might expect something more like the following (various overhead
properties like reg/#address-cells etc. elided for simplicity):
disp: display {
width-mm = <...>;
height-mm = <...>;
modes {
mode@0 {
/* 1920x1080p24 */
clock = <52000000>;
xres = <1920>;
yres = <1080>;
left-margin = <25>;
right-margin = <25>;
hsync-len = <25>;
lower-margin = <2>;
upper-margin = <2>;
vsync-len = <2>;
hsync-active-high;
};
mode@1 {
};
};
};
display-connector {
display = <&disp>;
// interface-specific properties such as pixel format here
};
Finally, have you considered just using an EDID instead of creating
something custom? I know that creating an EDID is harder than writing a
few simple properties into a DT, but EDIDs have the following advantages:
a) They're already standardized and very common.
b) They allow other information such as a display's HDMI audio
capabilities to be represented, which can then feed into an ALSA driver.
c) The few LCD panel datasheets I've seen actually quote their
specification as an EDID already, so deriving the EDID may actually be easy.
d) People familiar with displays are almost certainly familiar with
EDID's mode representation. There are many ways of representing display
modes (sync position vs. porch widths, htotal specified rather than
specifying all the components and hence htotal being calculated etc.).
Not everyone will be familiar with all representations. Conversion
errors are less likely if the target is EDID's familiar format.
e) You'll end up with exactly the same data as if you have a DDC-based
external monitor rather than an internal panel, so you end up getting to
a common path in display handling code much more quickly.
^ permalink raw reply
* Re: [PATCH v2] of: Add videomode helper
From: Stephen Warren @ 2012-08-02 19:43 UTC (permalink / raw)
To: Rob Herring
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Sascha Hauer,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org,
Laurent Pinchart, kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
In-Reply-To: <4FF5A9FB.7010004-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
On 07/05/2012 08:51 AM, Rob Herring wrote:
> On 07/04/2012 02:56 AM, Sascha Hauer wrote:
>> This patch adds a helper function for parsing videomodes from the devicetree.
>> The videomode can be either converted to a struct drm_display_mode or a
>> struct fb_videomode.
>> diff --git a/Documentation/devicetree/bindings/video/displaymode b/Documentation/devicetree/bindings/video/displaymode
>> +Example:
>> +
>> + display@0 {
>> + /* 1920x1080p24 */
>> + clock = <52000000>;
>
> Should this use the clock binding? You probably need both constraints
> and clock binding though.
I don't think the clock binding is appropriate here. This binding
specifies a desired video mode, and should specify just the expected
clock frequency for that mode. Perhaps any tolerance imposed by the
specification used to calculate the mode timing could be specified too,
but the allowed tolerance (for a mode to be recognized by the dispaly)
is more driven by the receiving device than the transmitting device.
The clock bindings should come into play in the display controller that
sends a signal in that display mode. Something like:
mode: hd1080p {
clock = <52000000>;
xres = <1920>;
yres = <1080>;
....
};
display-controller {
pixel-clock-source = <&clk ...>; // common clock bindings here
default-mode = <&mode>;
> Often you don't know the frequency up front and/or have limited control
> of the frequency (i.e. integer dividers). Then you have to adjust the
> margins to get the desired refresh rate. To do that, you need to know
> the ranges of values a panel can support. Perhaps you just assume you
> can increase the right-margin and lower-margins as I think you will hit
> pixel clock frequency max before any limit on margins.
I think this is more usually dealt with in HW design and matching
components.
The PLL/... feeding the display controller is going to have some known
specifications that imply which pixel clocks it can generate. HW
designers will pick a panel that accepts a clock the display controller
can generate. The driver for the display controller will just generate
as near of a pixel clock as it can to the rate specified in the mode
definition. I believe it'd be unusual for a display driver to start
fiddling with front-back porch (or margin) widths to munge the timing;
that kind of thing probably worked OK with analog displays, but with
digital displays where each pixel is clocked even in the margins, I
think that would cause more problems than it solved.
Similarly for external displays, the display controller will just pick
the nearest clock it can. If it can't generate a close enough clock, it
will just refuse to set the mode. In fact, a display controller driver
would typically validate this when the set of legal modes was enumerated
when initially detecting the display, so user-space typically wouldn't
even request invalid modes.
^ permalink raw reply
* Re: viafb on Clevo M5x0V laptop (+VT1631L)
From: Florian Tobias Schandinat @ 2012-08-02 20:46 UTC (permalink / raw)
To: Ondrej Zary; +Cc: linux-fbdev, Kernel development list
In-Reply-To: <201208022003.28404.linux@rainbow-software.org>
Hi Ondrej,
On 08/02/2012 06:03 PM, Ondrej Zary wrote:
> Hello,
> I've got a Clevo M5x0V laptop which has VIA PM800 chipset (1106:3118) and
> 1280x768 LCD panel connected using VT1631L LVDS transmitter. There's a
> schematic of this laptop available on the web which shows that VT1631L's I2C
> port is not connected anywhere...
>
> viafb does not work properly - LCD panel (and backlight) is turned off upon
> module load and there's 640x480 signal on VGA connector (even when monitor
> was not connected during module load).
Yes, that's the default (640x480-60 on the VGA output) that VIA selected
when they wrote the module. I tried to add some auto detect mechanism,
but I guess in your case it wouldn't help, even if it were complete. The
traditional way to use viafb is via module parameters as described in
Documentation/fb/viafb.txt
In your case something like this might work:
viafb_mode\x1280x768 viafb_lcd_panel_id=3 viafb_active_dev=LCD
Additionally it might be required to add viafb_lcd_port with any of
DVP0, DVP1, DFP_HIGHLOW, DFP_HIGH, DFP_LOW.
> Looking at the code, there's almost nothing for VT1631. What needs to be done
> in order for the LCD to work? Xorg seems to work using openchrome (but it is
> not able to restore the card on resume from S3):
> (II) CHROME(0): Unable to get panel size from EDID. Return code: 0
> (II) CHROME(0): ViaPanelGetNativeModeFromScratchPad
> (II) CHROME(0): Native Panel Resolution is 1280x768
Interesting that they get the panel resolution from the scratch pad. The
official documentation does not contain any useful information about it
(and openchrome didn't work on any of my LCD devices that I tested,
hence I didn't care whether they did have any code in that area)
Maybe the above module parameters will help you, otherwise more
investigation will be required.
Best regards,
Florian Tobias Schandinat
>
>
> This is from kernel 3.5 with VIAFB_DEBUG and VIAFB_WARN enabled:
>
> [ 153.051061] VIA Graphics Integration Chipset framebuffer 2.4 initializing
> [ 153.051765] viafb 0000:01:00.0: power state changed by ACPI to D0
> [ 153.051773] viafb 0000:01:00.0: power state changed by ACPI to D0
> [ 153.053535] Device ID = 3259
> [ 153.053553] FB Size = 4000
> [ 153.059984] VIAFB PCI Probe!!
> [ 153.060044] viafb: Probing I2C bus 0x26
> [ 153.092761] viafb: Probing I2C bus 0x31
> [ 155.588019] viafb: Probing I2C bus 0x2C
> [ 155.619016] viafb: Finished I2C bus probing
> [ 155.619021] parse_lcd_port: viafb_lcd_port:,interface:0
> [ 155.619023] parse_dvi_port: viafb_dvi_port:,interface:0
> [ 155.713224] TMDS Chip = 0
> [ 155.713227] viafb_init_dvi_size()
> [ 155.713229] viafb_dvi_sense!!
> [ 155.714415] viafb_dvi_query_EDID!!
> [ 155.716820] viafb_dvi_query_EDID!!
> [ 155.719179] viafb_lvds_identify_vt1636.
> [ 155.812018] viafb_lvds_identify_vt1636.
> [ 155.814383] viafb_init_lcd_size()
> [ 155.814384] fp_get_panel_id()
> [ 155.814387] LVDS Chip = 0
> [ 155.814389] LVDS1 output_interface = 6
> [ 155.814391] LVDS2 output_interface = 6
> [ 155.814405] viafb_check_var!
> [ 155.815250] fbcon: Via (fb0) is primary device
> [ 155.815257] viafb_open!
> [ 155.818608] viafb_set_par!
> [ 155.818720] via_set_primary_pitch(0x00000A00)
> [ 155.818723] via_set_secondary_pitch(0x00000A00)
> [ 155.818727] via_set_primary_color_depth(24)
> [ 155.818730] via_set_secondary_color_depth(24)
> [ 155.818788] viafb_pan_display, address = 0
> [ 155.818789] via_set_primary_address(0x00000000)
> [ 155.818793] via_set_secondary_address(0x00000000)
> [ 155.819020] viafb_pan_display, address = 0
> [ 155.819021] via_set_primary_address(0x00000000)
> [ 155.819025] via_set_secondary_address(0x00000000)
> [ 155.819044] viafb 2D engine: imageblit
> ...many imageblits...
> [ 155.826516] Console: switching to colour frame buffer device 80x30
> [ 155.826522] viafb_pan_display, address = 0
> [ 155.826523] via_set_primary_address(0x00000000)
> [ 155.826527] via_set_secondary_address(0x00000000)
> ...many imageblits...
> [ 155.833889] fb0: Via frame buffer device 640x480-32bpp
>
>
^ permalink raw reply
* Re: viafb on Clevo M5x0V laptop (+VT1631L)
From: Ondrej Zary @ 2012-08-02 21:13 UTC (permalink / raw)
To: Florian Tobias Schandinat; +Cc: linux-fbdev, Kernel development list
In-Reply-To: <501AE71A.80903@gmx.de>
On Thursday 02 August 2012 22:46:18 Florian Tobias Schandinat wrote:
> Hi Ondrej,
>
> On 08/02/2012 06:03 PM, Ondrej Zary wrote:
> > Hello,
> > I've got a Clevo M5x0V laptop which has VIA PM800 chipset (1106:3118) and
> > 1280x768 LCD panel connected using VT1631L LVDS transmitter. There's a
> > schematic of this laptop available on the web which shows that VT1631L's
> > I2C port is not connected anywhere...
> >
> > viafb does not work properly - LCD panel (and backlight) is turned off
> > upon module load and there's 640x480 signal on VGA connector (even when
> > monitor was not connected during module load).
>
> Yes, that's the default (640x480-60 on the VGA output) that VIA selected
> when they wrote the module. I tried to add some auto detect mechanism,
> but I guess in your case it wouldn't help, even if it were complete. The
> traditional way to use viafb is via module parameters as described in
> Documentation/fb/viafb.txt
> In your case something like this might work:
> viafb_mode\x1280x768 viafb_lcd_panel_id=3 viafb_active_dev=LCD
Thank you very much, it works with these parameters! Looks like viafb can wake
up the chip from S3, only the colors are messed up (X<->VT switch fixes
that).
> Additionally it might be required to add viafb_lcd_port with any of
> DVP0, DVP1, DFP_HIGHLOW, DFP_HIGH, DFP_LOW.
>
> > Looking at the code, there's almost nothing for VT1631. What needs to be
> > done in order for the LCD to work? Xorg seems to work using openchrome
> > (but it is not able to restore the card on resume from S3):
> > (II) CHROME(0): Unable to get panel size from EDID. Return code: 0
> > (II) CHROME(0): ViaPanelGetNativeModeFromScratchPad
> > (II) CHROME(0): Native Panel Resolution is 1280x768
>
> Interesting that they get the panel resolution from the scratch pad. The
> official documentation does not contain any useful information about it
> (and openchrome didn't work on any of my LCD devices that I tested,
> hence I didn't care whether they did have any code in that area)
> Maybe the above module parameters will help you, otherwise more
> investigation will be required.
Looking at openchrome and viafb code - the code is already present in viafb in
fp_id_to_vindex() function in lcd.c. It even finds correct panel_id for me
(3). But it looks like that the result is not used.
--
Ondrej Zary
^ permalink raw reply
* Re: [RFC][PATCH v3 1/3] runtime interpreted power sequences
From: Alex Courbot @ 2012-08-03 1:15 UTC (permalink / raw)
To: Mark Brown
Cc: Stephen Warren, Stephen Warren, Simon Glass, Grant Likely,
Rob Herring, Greg Kroah-Hartman, Arnd Bergmann,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
Thierry Reding
In-Reply-To: <20120802181111.GM4537-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org>
On Fri 03 Aug 2012 03:11:12 AM JST, Mark Brown wrote:
> On Thu, Aug 02, 2012 at 10:21:57AM +0200, Thierry Reding wrote:
>> On Thu, Aug 02, 2012 at 05:00:13PM +0900, Alex Courbot wrote:
>
>>> The problem is, how do we turn these phandles into the resource of
>>> interest. The type of the resource can be infered by the name of the
>>> property. The hard part is resolving the resource from the phandle -
>>> it seems like the API just does not allow to do this. GPIO has
>>> of_get_named_gpio, but AFAIK there are no equivalent for regulator
>>> consumer and PWM: the only way to use the DT with them is through
>>> get_regulator and get_pwm which work at the device level.
>
>>> Or is there a way that I overlooked?
>
>> No, you are right. Perhaps we should add exported functions that do the
>> equivalent of of_pwm_request() or the regulator_dev_lookup() and
>> of_get_regulator() pair.
>
> I missed some of the earlier bits of the thread here but why can't we do
> device based lookups?
That is because the phandles would not be properties of the device node
but rather of its sub-nodes:
backlight {
compatible = "pwm-backlight";
...
power-on-sequence {
step@0 {
regulator = <&backlight_reg>;
enable;
};
So here simply using regulator_get on the backlight device would not work.
Alex.
^ permalink raw reply
* Re: [PATCH v2] of: Add videomode helper
From: Sascha Hauer @ 2012-08-03 7:38 UTC (permalink / raw)
To: Stephen Warren
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW@public.gmane.org,
Laurent Pinchart, kernel-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org
In-Reply-To: <501AD68C.1000904-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
Hi Stephen,
On Thu, Aug 02, 2012 at 01:35:40PM -0600, Stephen Warren wrote:
> On 07/04/2012 01:56 AM, Sascha Hauer wrote:
> > This patch adds a helper function for parsing videomodes from the devicetree.
> > The videomode can be either converted to a struct drm_display_mode or a
> > struct fb_videomode.
>
> > diff --git a/Documentation/devicetree/bindings/video/displaymode b/Documentation/devicetree/bindings/video/displaymode
>
> > +Required properties:
> > + - xres, yres: Display resolution
> > + - left-margin, right-margin, hsync-len: Horizontal Display timing parameters
> > + in pixels
> > + upper-margin, lower-margin, vsync-len: Vertical display timing parameters in
> > + lines
>
> Perhaps bike-shedding, but...
>
> For the margin property names, wouldn't it be better to use terminology
> more commonly used for video timings rather than Linux FB naming. In
> other words naming like:
>
> hactive, hsync-len, hfront-porch, hback-porch?
Can do. Just to make sure:
hactive = xres
hsync-len = hsync-len
hfront-porch = right-margin
hback-porch = left-margin
?
>
> This node appears to describe a video mode, not a display, hence the
> node name seems wrong.
>
> Many displays can support multiple different video modes. As mentioned
> elsewhere, properties like display width/height are properties of the
> display not the mode.
>
> So, I might expect something more like the following (various overhead
> properties like reg/#address-cells etc. elided for simplicity):
>
> disp: display {
> width-mm = <...>;
> height-mm = <...>;
> modes {
> mode@0 {
> /* 1920x1080p24 */
> clock = <52000000>;
> xres = <1920>;
> yres = <1080>;
> left-margin = <25>;
> right-margin = <25>;
> hsync-len = <25>;
> lower-margin = <2>;
> upper-margin = <2>;
> vsync-len = <2>;
> hsync-active-high;
> };
> mode@1 {
> };
> };
> };
Ok, we can do this.
>
> display-connector {
> display = <&disp>;
> // interface-specific properties such as pixel format here
> };
>
> Finally, have you considered just using an EDID instead of creating
> something custom? I know that creating an EDID is harder than writing a
> few simple properties into a DT, but EDIDs have the following advantages:
I have considered using EDID and I also tried it. It's painful. There
are no (open) tools available for creating EDID. That's something we
could change of course. Then when generating a devicetree there is
always an extra step involved creating the EDID blob. Once the EDID
blob is in devicetree it is not parsable anymore by mere humans, so
to see what we've got there is another tool involved to generate a
readable form again.
>
> a) They're already standardized and very common.
Indeed, that's a big plus for EDID. I have no intention of removing EDID
data from the devicetree. There are situations where EDID is handy, for
example when you get EDID data provided by your vendor.
>
> b) They allow other information such as a display's HDMI audio
> capabilities to be represented, which can then feed into an ALSA driver.
>
> c) The few LCD panel datasheets I've seen actually quote their
> specification as an EDID already, so deriving the EDID may actually be easy.
>
> d) People familiar with displays are almost certainly familiar with
> EDID's mode representation. There are many ways of representing display
> modes (sync position vs. porch widths, htotal specified rather than
> specifying all the components and hence htotal being calculated etc.).
> Not everyone will be familiar with all representations. Conversion
> errors are less likely if the target is EDID's familiar format.
You seem to think about a different class of displays for which EDID
indeed is a better way to handle. What I have to deal with here mostly
are dumb displays which:
- can only handle their native resolution
- Have no audio capabilities at all
- come with a datasheet which specify a min/typ/max triplet for
xres,hsync,..., often enough they are scanned to pdf from some previously
printed paper.
These displays are very common on embedded devices, probably that's the
reason I did not even think about the possibility that a single display
might have different modes.
>
> e) You'll end up with exactly the same data as if you have a DDC-based
> external monitor rather than an internal panel, so you end up getting to
> a common path in display handling code much more quickly.
All we have in our display driver currently is:
edidp = of_get_property(np, "edid", &imxpd->edid_len);
if (edidp) {
imxpd->edid = kmemdup(edidp, imxpd->edid_len, GFP_KERNEL);
} else {
ret = of_get_video_mode(np, &imxpd->mode, NULL);
if (!ret)
imxpd->mode_valid = 1;
}
Sascha
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
^ permalink raw reply
* [PATCH 0/5] use devm_ functions
From: Damien Cassou @ 2012-08-03 15:40 UTC (permalink / raw)
To: Florian Tobias Schandinat; +Cc: kernel-janitors, linux-fbdev, linux-kernel
In-Reply-To: <1343742860-16213-1-git-send-email-damien.cassou@lifl.fr>
These patches introduce devm_ functions in some video drivers. They have been
generated using Coccinelle (http://coccinelle.lip6.fr/) and the following
semantic patch:
virtual after_start
virtual returned
virtual returnedDup
virtual arg
virtual argDup
virtual all_args
virtual get
virtual nonull
virtual report
// ---------------------------------------------------------------------
// find functions
@plat depends on !after_start@
identifier i,pfn,rfn;
position p;
@@
struct platform_driver i@p = {
.probe = pfn,
.remove = (<+...rfn...+>),
};
// ---------------------------------------------------------------------
// set up iteration
@initialize:ocaml@
let reporting = ref false
type ret UseReturned | UseReturned2 of string | UseReturnedDup | UseReturnedNN
| UseArg | UseArgDup | UseArgNN | UseAllArgs | UseGet
let add pfn rfn alloc free devm_alloc file rule let it = new iteration() in
it#set_files [file];
it#add_virtual_rule After_start;
(match rule with
UseReturned -> it#add_virtual_rule Returned
| UseReturnedNN -> it#add_virtual_rule Returned; it#add_virtual_rule Nonull
| UseReturnedDup -> it#add_virtual_rule ReturnedDup
| UseReturned2(free) -> it#add_virtual_rule Returned;
it#add_virtual_identifier Second_free free
| UseArg -> it#add_virtual_rule Arg
| UseArgNN -> it#add_virtual_rule Arg; it#add_virtual_rule Nonull
| UseArgDup -> it#add_virtual_rule ArgDup
| UseAllArgs -> it#add_virtual_rule All_args
| UseGet -> it#add_virtual_rule Get);
if !reporting then it#add_virtual_rule Report;
if not (pfn="") then it#add_virtual_identifier Pfn pfn;
if not (rfn="") then it#add_virtual_identifier Rfn rfn;
if not (alloc="") then it#add_virtual_identifier Alloc alloc;
if not (free="") then it#add_virtual_identifier Free free;
if not (devm_alloc="") then it#add_virtual_identifier Devm_alloc devm_alloc;
it#register()
@script:ocaml depends on report && !after_start@
@@
reporting := true
@script:ocaml@
pfn << plat.pfn;
rfn << plat.rfn;
p << plat.p;
@@
let file = (List.hd p).file in
add pfn rfn "kmalloc" "kfree" "devm_kzalloc" file UseReturned;
add pfn rfn "kzalloc" "kfree" "devm_kzalloc" file UseReturned;
add pfn rfn "ioremap" "iounmap" "devm_ioremap" file UseReturned;
add pfn rfn "ioremap_nocache" "iounmap" "devm_ioremap_nocache" file
UseReturned;
add pfn rfn "clk_get" "clk_put" "devm_clk_get" file UseReturnedDup;
add pfn rfn "clk_get" "clk_put" "devm_clk_get_bad" file UseReturnedNN;
add pfn rfn "usb_get_phy" "usb_put_phy" "devm_usb_get_phy" file UseReturned;
add pfn rfn "pinctrl_get" "pinctrl_put" "devm_pinctrl_get" file UseReturned;
add pfn rfn "pinctrl_get_select_default" "pinctrl_put"
"devm_pinctrl_get_select_default" file UseReturned;
add pfn rfn "regulator_get" "regulator_put" "devm_regulator_get"
file UseReturned;
add pfn rfn "regulator_bulk_get" "regulator_bulk_free"
"devm_regulator_bulk_get" file UseAllArgs;
add pfn rfn "gpio_request" "gpio_free" "devm_gpio_request" file UseArg;
add pfn rfn "gpio_request_one" "gpio_free" "devm_gpio_request_one" file UseArg;
(*
add pfn rfn "request_irq" "free_irq" "devm_request_irq" file UseArg;
add pfn rfn "request_threaded_irq" "free_irq" "devm_request_threaded_irq" file
UseArg;
*)
add pfn rfn "dma_alloc_coherent" "dma_free_coherent" "dmam_alloc_coherent"
file UseReturnedDup;
add pfn rfn "dma_alloc_noncoherent" "dma_free_noncoherent"
"dmam_alloc_noncoherent" file UseReturnedDup;
add pfn rfn "dma_alloc_coherent" "dma_free_coherent" "dmam_alloc_coherent_bad"
file UseReturnedNN;
add pfn rfn "dma_alloc_noncoherent" "dma_free_noncoherent"
"dmam_alloc_noncoherent_bad" file UseReturnedNN;
(* several possibilities... *)
add pfn rfn "request_region" "release_region" "devm_request_region" file
UseGet;
add pfn rfn "request_mem_region" "release_mem_region"
"devm_request_mem_region" file UseGet;
add pfn rfn "request_region" "release_region" "devm_request_region" file
UseArg;
add pfn rfn "request_mem_region" "release_mem_region"
"devm_request_mem_region" file UseArg;
(* fix a bug at the same time *)
add pfn rfn "request_region" "release_resource" "devm_request_region" file
(UseReturned2("kfree"));
add pfn rfn "request_mem_region" "release_resource"
"devm_request_mem_region" file (UseReturned2("kfree"));
add pfn rfn "ioport_map" "ioport_unmap" "devm_ioport_map" file UseReturned
// ---------------------------------------------------------------------
// process the initial definition of the probe function
@preprobe@
identifier virtual.pfn;
position p;
@@
pfn@p(...) { ... }
@probe@
identifier pfn;
position preprobe.p;
@@
pfn@p(...) { ... }
@labelled_return@
identifier probe.pfn,l;
expression e;
@@
pfn(...) { <+... l: return e; ...+> }
// ---------------------------------------------------------------------
// transform functions where free uses the result
@prb depends on returned exists@
identifier probe.pfn,pdev,virtual.alloc,virtual.free,virtual.second_free;
expression x,y,e,a;
position p1,p2,p3;
type T1,T2,T3;
@@
pfn(struct platform_device *pdev) { ... when any
x = (T1)alloc@p1(a,...)
<... when strict
when any
when forall
(
y = x;
... when != y = e
when != &y
free@p2(...,(T2)y,...);
... when != x
when != y
second_free@p3(...,(T3)y,...);
|
y = x;
... when != y = e
when != &y
free@p2(...,(T2)y,...);
|
free@p2(...,(T2)x,...);
... when != x
second_free@p3(...,(T3)x,...);
|
free@p2(...,(T2)x,...);
)
...>
}
@script:ocaml@
a << prb.a;
@@
if a = "NULL" then Coccilib.include_match false
@reme exists@
identifier virtual.rfn,virtual.free;
expression prb.x,prb.y;
type T;
@@
rfn(...) { ... free(...,(T)\(x\|y\),...); ... }
@rem depends on reme@
identifier virtual.rfn,virtual.free,virtual.second_free;
expression prb.x,prb.y;
position p4,p5;
type T,T1;
@@
rfn(...) {
<... when strict
(
free@p4(...,(T)\(x\|y\),...);
... when != x
second_free@p5(...,(T1)\(x\|y\),...);
|
free@p4(...,(T)\(x\|y\),...);
)
...>
}
@bad@
identifier virtual.free;
expression prb.x,prb.y;
position p != {prb.p2,rem.p4};
type T;
@@
free@p(...,(T)\(x\|y\),...)
@modif depends on rem && !bad && !report@
expression x;
identifier prb.pdev,virtual.alloc,virtual.free,virtual.devm_alloc;
identifier virtual.second_free;
expression list args;
position prb.p1,prb.p2,prb.p3,rem.p4,rem.p5;
type T;
@@
(
- free@p2(...);
|
- second_free@p3(...);
|
- free@p4(...);
|
- second_free@p5(...);
|
x - alloc@p1(
+ devm_alloc(&pdev->dev,
args)
|
x - (T)alloc@p1(
+ (T)devm_alloc(&pdev->dev,
args)
)
@script:python depends on rem && !bad && report@
p1 << prb.p1;
alloc << virtual.devm_alloc;
@@
msg = "WARNING opportunity for %s" % (alloc)
coccilib.report.print_report(p1[0], msg)
// ---------------------------------------------------------------------
// transform functions where free uses the result
// special case for clk where don't add &pdev->dev
@prbdup depends on returnedDup exists@
identifier probe.pfn,pdev,virtual.alloc,virtual.free,virtual.second_free;
expression x,y,e;
expression list args;
position p1,p2,p3;
type T1,T2,T3;
@@
pfn(struct platform_device *pdev) { ... when any
x = (T1)alloc@p1(&pdev->dev,args)
<... when strict
when any
when forall
(
y = x;
... when != y = e
when != &y
free@p2(...,(T2)y,...);
... when != x
when != y
second_free@p3(...,(T3)y,...);
|
y = x;
... when != y = e
when != &y
free@p2(...,(T2)y,...);
|
free@p2(...,(T2)x,...);
... when != x
second_free@p3(...,(T3)x,...);
|
free@p2(...,(T2)x,...);
)
...>
}
@remdupe exists@
identifier virtual.rfn,virtual.free;
expression prbdup.x,prbdup.y;
type T;
@@
rfn(...) { ... free(...,(T)\(x\|y\),...); ... }
@remdup depends on remdupe@
identifier virtual.rfn,virtual.free,virtual.second_free;
expression prbdup.x,prbdup.y;
position p4,p5;
type T,T1;
@@
rfn(...) {
<... when strict
(
free@p4(...,(T)\(x\|y\),...);
... when != x
second_free@p5(...,(T1)\(x\|y\),...);
|
free@p4(...,(T)\(x\|y\),...);
)
...>
}
@baddup@
identifier virtual.free;
expression prbdup.x,prbdup.y;
position p != {prbdup.p2,remdup.p4};
type T;
@@
free@p(...,(T)\(x\|y\),...)
@modifdup depends on remdup && !baddup && !report@
expression x;
identifier virtual.alloc,virtual.free,virtual.devm_alloc;
identifier virtual.second_free;
expression list args;
position prbdup.p1,prbdup.p2,prbdup.p3,remdup.p4,remdup.p5;
type T;
@@
(
- free@p2(...);
|
- second_free@p3(...);
|
- free@p4(...);
|
- second_free@p5(...);
|
x - alloc@p1
+ devm_alloc
(args)
|
x - (T)alloc@p1
+ (T)devm_alloc
(args)
)
@script:python depends on remdup && !baddup && report@
p1 << prbdup.p1;
alloc << virtual.devm_alloc;
@@
msg = "WARNING opportunity for %s" % (alloc)
coccilib.report.print_report(p1[0], msg)
// ---------------------------------------------------------------------
// transform functions where free uses the first argument
@prbx depends on arg exists@
identifier probe.pfn,pdev,virtual.alloc,virtual.free;
expression x,y,e;
expression list args;
position p1,p2;
@@
pfn(struct platform_device *pdev) { ... when any
alloc@p1(x,args)
<... when strict
when any
when forall
(
y = x;
... when != y = e
when != &y
free@p2(y,...);
|
free@p2(x,...)
)
...>
}
@script:ocaml@
x << prbx.x;
@@
if x = "NULL" then Coccilib.include_match false
@remxe exists@
identifier virtual.rfn, virtual.free;
expression prbx.x,prbx.y;
@@
rfn(...) { ... free(\(x\|y\),...); ... }
@remx depends on remxe@
identifier virtual.rfn, virtual.free;
expression prbx.x,prbx.y;
position p3;
@@
rfn(...) {
<... when strict
free@p3(\(x\|y\),...)
...>
}
@badx@
identifier virtual.free;
expression prbx.x,prbx.y;
position p != {prbx.p2,remx.p3};
@@
free@p(\(x\|y\),...)
@modifx depends on remx && !badx && !report@
expression x;
identifier prbx.pdev,virtual.alloc,virtual.free,virtual.devm_alloc;
expression list args;
position prbx.p1,prbx.p2,remx.p3;
@@
(
- free@p2(...);
|
- free@p3(...);
|
- alloc@p1(
+ devm_alloc(&pdev->dev,
x,args)
)
@script:python depends on remx && !badx && report@
p1 << prbx.p1;
alloc << virtual.devm_alloc;
@@
msg = "WARNING opportunity for %s" % (alloc)
coccilib.report.print_report(p1[0], msg)
// ---------------------------------------------------------------------
// transform functions where free uses the first argument
@prbxdup depends on argDup exists@
identifier probe.pfn,pdev,virtual.alloc,virtual.free;
expression y,e;
expression list args;
position p1,p2;
@@
pfn(struct platform_device *pdev) { ... when any
alloc@p1(&pdev->dev,args)
<... when strict
when any
when forall
(
y = &pdev->dev;
... when != y = e
when != &y
free@p2(y,...);
|
free@p2(&pdev->dev,...)
)
...>
}
@remxedup exists@
identifier virtual.rfn, virtual.free, prbxdup.pdev;
expression prbxdup.y;
@@
rfn(...) { ... free(\(&pdev->dev\|y\),...); ... }
@remxdup depends on remxedup@
identifier virtual.rfn, virtual.free, prbxdup.pdev;
expression prbxdup.y;
position p3;
@@
rfn(...) {
<... when strict
free@p3(\(&pdev->dev\|y\),...)
...>
}
@badxdup@
identifier virtual.free,prbxdup.pdev;
expression prbxdup.y;
position p != {prbxdup.p2,remxdup.p3};
@@
free@p(\(&pdev->dev\|y\),...)
@modifxdup depends on remxdup && !badxdup && !report@
identifier virtual.alloc,virtual.free,virtual.devm_alloc;
expression list args;
position prbxdup.p1,prbxdup.p2,remxdup.p3;
@@
(
- free@p2(...);
|
- free@p3(...);
|
- alloc@p1
+ devm_alloc
(args)
)
@script:python depends on remxdup && !badxdup && report@
p1 << prbxdup.p1;
alloc << virtual.devm_alloc;
@@
msg = "WARNING opportunity for %s" % (alloc)
coccilib.report.print_report(p1[0], msg)
// ---------------------------------------------------------------------
// transform functions where free uses all arguments
@prbax depends on all_args exists@
identifier probe.pfn,pdev,virtual.alloc,virtual.free;
expression list x;
position p1,p2;
@@
pfn(struct platform_device *pdev) { ... when any
alloc@p1(x)
<... when strict
when any
when forall
free@p2(x)
...>
}
@remaxe exists@
identifier virtual.rfn, virtual.free;
expression list prbax.x;
@@
rfn(...) { ... free(x); ... }
@remax depends on remaxe@
identifier virtual.rfn, virtual.free;
expression list prbax.x;
position p3;
@@
rfn(...) {
<... when strict
free@p3(x)
...>
}
@badax@
identifier virtual.free;
expression list prbax.x;
position p != {prbax.p2,remax.p3};
@@
free@p(x)
@modifax depends on remax && !badax && !report@
identifier prbax.pdev,virtual.alloc,virtual.free,virtual.devm_alloc;
expression list x;
position prbax.p1,prbax.p2,remax.p3;
@@
(
- free@p2(...);
|
- free@p3(...);
|
- alloc@p1(
+ devm_alloc(&pdev->dev,
x)
)
@script:python depends on remax && !badax && report@
p1 << prbax.p1;
alloc << virtual.devm_alloc;
@@
msg = "WARNING opportunity for %s" % (alloc)
coccilib.report.print_report(p1[0], msg)
// ---------------------------------------------------------------------
// transform functions where free uses the result of platform_get_resource
@prbg depends on get exists@
identifier probe.pfn,pdev,virtual.alloc,virtual.free;
expression x,y,e;
expression list args;
position p1,p2;
@@
pfn(struct platform_device *pdev) { ... when any
alloc@p1(x,args)
<... when strict
when any
when forall
(
y = x;
... when != y = e
when != &y
free@p2(y,...);
|
free@p2(x,...)
)
...>
}
@remge exists@
identifier virtual.rfn, virtual.free;
identifier z;
identifier pdev;
expression e,n;
@@
rfn(struct platform_device *pdev) { ... when any
z = platform_get_resource(pdev, IORESOURCE_MEM, n)
... when != z = e
when != &z
free(z->start,...)
...
}
@remg depends on remge@
identifier virtual.rfn, virtual.free;
identifier z;
identifier pdev;
position p3;
expression e,n;
@@
rfn(struct platform_device *pdev) {
<... when strict
z = platform_get_resource(pdev, IORESOURCE_MEM, n)
... when strict
when != z = e
when != &z
free@p3(z->start,...)
...>
}
@badg@
identifier virtual.free;
position p != {prbg.p2,remg.p3};
@@
free@p(...)
@modifg depends on remg && !badg && !report@
expression x;
identifier prbg.pdev,virtual.alloc,virtual.free,virtual.devm_alloc;
expression list args;
position prbg.p1,prbg.p2,remg.p3;
@@
(
- free@p2(...);
|
- free@p3(...);
|
- alloc@p1(
+ devm_alloc(&pdev->dev,
x,args)
)
@script:python depends on remg && !badg && report@
p1 << prbg.p1;
alloc << virtual.devm_alloc;
@@
msg = "WARNING opportunity for %s" % (alloc)
coccilib.report.print_report(p1[0], msg)
// ---------------------------------------------------------------------
// cleanup, if the drvdata was only used to enable the free
// probably only relevant for kmalloc/kzalloc
@dclean depends on modif || modifdup || modifx || modifxdup || modifax || modifg@
identifier virtual.rfn, pdev, i;
type T;
@@
rfn(struct platform_device *pdev) { ...
(
- T i = platform_get_drvdata(pdev);
|
- T i = dev_get_drvdata(&pdev->drv);
|
- T i;
... when != i
(
- i = platform_get_drvdata(pdev);
|
- i = dev_get_drvdata(&pdev->drv);
)
)
... when != i
}
@rclean depends on modif || modifdup || modifx || modifxdup || modifax || modifg@
identifier virtual.rfn, pdev, i;
type T;
@@
rfn(struct platform_device *pdev) { ...
(
- T i = platform_get_resource(pdev,...);
|
- T i;
... when != i
- i = platform_get_resource(pdev,...);
)
... when != i
}
// ---------------------------------------------------------------------
// cleanup empty ifs, etc
@depends on modif || modifdup || modifx || modifxdup || modifax || modifg@
identifier probe.pfn;
@@
pfn(...) { <...
- if (...) {}
...> }
@depends on modif || modifdup || modifx || modifxdup || modifax || modifg@
identifier virtual.rfn;
@@
rfn(...) { <...
- if (...) {}
...> }
@depends on modif || modifdup || modifx || modifxdup || modifax || modifg@
identifier probe.pfn;
expression ret,e;
@@
pfn(...) { <...
+ return
- ret e;
- return ret;
...> }
@depends on modif || modifdup || modifx || modifxdup || modifax || modifg@
identifier virtual.rfn;
expression ret,e;
@@
rfn(...) { <...
+ return
- ret e;
- return ret;
...> }
// ---------------------------------------------------------------------
// this is likely to leave dead code, if l: is preceded by a return
// because we are control-flow based, there is no way to match on that
@depends on labelled_return && (modif || modifdup || modifx || modifxdup || modifax || modifg)@
identifier l,l1,l2;
expression e;
statement S;
identifier probe.pfn;
identifier i;
statement S1,S2;
@@
pfn(...) { <...
- goto l;
+ goto l2;
...
-l:
<... when != S
when any
l1:
...>
l2:
(
(<+...i...+>);
|
if (...) S1 else S2
|
while (...) S1
|
for (...;...;...) S1
|
return e;
)
...> }
@depends on !labelled_return && (modif || modifdup || modifx || modifxdup || modifax || modifg)@
identifier l,l1,l2;
expression e;
statement S;
identifier probe.pfn;
identifier i;
statement S1,S2;
@@
pfn(...) { <...
(
- goto l;
+ goto l2;
...
-l:
<... when != S
when any
l1:
...>
l2:
(
(<+...i...+>);
|
if (...) S1 else S2
|
while (...) S1
|
for (...;...;...) S1
)
|
- goto l;
+ return e;
...
-l:
<... when != S
when any
l1:
...>
return e;
)
...> }
@depends on !labelled_return && (modif || modifdup || modifx || modifxdup || modifax || modifg)@
expression e1,e2;
identifier probe.pfn;
@@
pfn(...) { <...
-e1 = e2;
-return e1;
+return e2;
...> }
^ permalink raw reply
* [PATCH 1/5] drivers/video/epson1355fb.c: use devm_ functions
From: Damien Cassou @ 2012-08-03 15:40 UTC (permalink / raw)
To: Christopher Hoover
Cc: kernel-janitors, Florian Tobias Schandinat, linux-fbdev,
linux-kernel
In-Reply-To: <1344008414-2894-1-git-send-email-damien.cassou@lifl.fr>
From: Damien Cassou <damien.cassou@lifl.fr>
The various devm_ functions allocate memory that is released when a driver
detaches. This patch uses these functions for data that is allocated in the
probe function of a platform device and is only freed in the remove function.
Signed-off-by: Damien Cassou <damien.cassou@lifl.fr>
---
drivers/video/epson1355fb.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/video/epson1355fb.c b/drivers/video/epson1355fb.c
index 68b9b51..246da1e 100644
--- a/drivers/video/epson1355fb.c
+++ b/drivers/video/epson1355fb.c
@@ -592,12 +592,8 @@ static int epson1355fb_remove(struct platform_device *dev)
if (info) {
fb_dealloc_cmap(&info->cmap);
- if (info->screen_base)
- iounmap(info->screen_base);
framebuffer_release(info);
}
- release_mem_region(EPSON1355FB_FB_PHYS, EPSON1355FB_FB_LEN);
- release_mem_region(EPSON1355FB_REGS_PHYS, EPSON1355FB_REGS_LEN);
return 0;
}
@@ -608,15 +604,18 @@ static int __devinit epson1355fb_probe(struct platform_device *dev)
u8 revision;
int rc = 0;
- if (!request_mem_region(EPSON1355FB_REGS_PHYS, EPSON1355FB_REGS_LEN, "S1D13505 registers")) {
+ if (!devm_request_mem_region(&dev->dev, EPSON1355FB_REGS_PHYS,
+ EPSON1355FB_REGS_LEN,
+ "S1D13505 registers")) {
printk(KERN_ERR "epson1355fb: unable to reserve "
"registers at 0x%0x\n", EPSON1355FB_REGS_PHYS);
rc = -EBUSY;
goto bail;
}
- if (!request_mem_region(EPSON1355FB_FB_PHYS, EPSON1355FB_FB_LEN,
- "S1D13505 framebuffer")) {
+ if (!devm_request_mem_region(&dev->dev, EPSON1355FB_FB_PHYS,
+ EPSON1355FB_FB_LEN,
+ "S1D13505 framebuffer")) {
printk(KERN_ERR "epson1355fb: unable to reserve "
"framebuffer at 0x%0x\n", EPSON1355FB_FB_PHYS);
rc = -EBUSY;
@@ -638,7 +637,8 @@ static int __devinit epson1355fb_probe(struct platform_device *dev)
}
info->pseudo_palette = default_par->pseudo_palette;
- info->screen_base = ioremap(EPSON1355FB_FB_PHYS, EPSON1355FB_FB_LEN);
+ info->screen_base = devm_ioremap(&dev->dev, EPSON1355FB_FB_PHYS,
+ EPSON1355FB_FB_LEN);
if (!info->screen_base) {
printk(KERN_ERR "epson1355fb: unable to map framebuffer\n");
rc = -ENOMEM;
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox