* Re: [PATCH 3/3] OMAPDSS: HDMI: Cache EDID
From: Jassi Brar @ 2012-06-28 15:30 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Andy Green, mythripk, linux-omap, linux-fbdev, n-dechesne,
patches
In-Reply-To: <1340896458.5037.131.camel@deskari>
On 28 June 2012 20:44, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Thu, 2012-06-28 at 18:43 +0530, Jassi Brar wrote:
>
>> A quick reaction of my guts say, we simply enable 5V/HPD_IRQ during
>> probe and disable during remove.
>> HDMI enable/disable via /sysfs/ and HPD (de)assertion, switch only
>> HDMI_PHY on/off.
>> The user selecting "Autodetect and Configure" option would then equate
>> to "(un)loading" of the HDMI driver.
>> Not to mean a trivial job.
>
> One more thing I realized while thinking about this:
>
> While it could be argued that the power draw from having the tpd12s015
> always enabled is very small, I think it could matter. If you consider a
> phone with HDMI output, it's likely that the phone is locked 99% of the
> time. When the phone is locked, there's no need to keep the HDMI HPD
> enabled. So this could add to a considerable amount of power wasted, if
> the HPD was always enabled.
>
> At least I can't figure out a reason why one would want the HPD to work
> when the phone is locked. Also, I have never used the HDMI output on my
> phone, so I'd be glad if it was totally powered off if it gave me more
> standby hours =).
>
Of course, I don't suggest imposing any hard rule here.
All I suggest is make it platform dependent and provide a way from
user-space too to enable/disable HPD.
^ permalink raw reply
* Re: [PATCH 3/3] OMAPDSS: HDMI: Cache EDID
From: Jassi Brar @ 2012-06-28 15:51 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Andy Green, mythripk, linux-omap, linux-fbdev, n-dechesne,
patches
In-Reply-To: <1340897244.5037.140.camel@deskari>
On 28 June 2012 20:57, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
> On Thu, 2012-06-28 at 20:44 +0530, Jassi Brar wrote:
>> it has, some user action should enable it while 'making the device
>> ready for new display'.
>> IOW, how do you envision an OMAP4 based tablet with HDMI port react to
>> display connections ?
>
> I guess this was covered in my mail about the phone's HDMI. If the
> tablet is unlocked, and I plug in a HDMI cable, I expect the device to
> do something. Either clone the display, or perhaps ask me what I want to
> do.
>
> So yes, HPD would be always enabled, when the tablet is active
> (unlocked).
>
OK, somehow I was under impression you didn't wanna spare even the 5V+ floating.
Though there could also be some option in settings to enable 5/HPD
only when the user is about the connect a display... so that the
activate window is even narrowed down. Anyway... I am glad we are in
sync.
>> > By the way, when the device is in system suspend, we surely won't detect
>> > the HPD even if we kept the HPD always enabled. So there we'll miss the
>> > HPD interrupt anyway, and the EDID cache would be invalid.
>> >
>> If omapdss already handles the possibility of display changed during
>> suspend, I think we should be good :)
>
> Hmm I'm not sure I understand what you mean. I was referring to your
> patch, which invalidated the EDID cache only on HPD interrupt when the
> cable is unplugged. And we'd miss that interrupt when the board is in
> system suspend, even if we otherwise kept the HPD interrupt always
> enabled.
>
I meant before stale-edid, we face potential problem of omapdss
behaving badly to the displays switched during suspend ?
^ permalink raw reply
* Re: [PATCH 3/3] OMAPDSS: HDMI: Cache EDID
From: Jassi Brar @ 2012-06-28 16:32 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Andy Green, mythripk, linux-omap, linux-fbdev, n-dechesne,
patches
In-Reply-To: <CAJe_Zhdnc8TOY8hDiuZfjto9fTWnT8piGpjczApzfLVMnCcE7w@mail.gmail.com>
On 28 June 2012 21:18, Jassi Brar <jaswinder.singh@linaro.org> wrote:
> On 28 June 2012 20:57, Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:
>>> > By the way, when the device is in system suspend, we surely won't detect
>>> > the HPD even if we kept the HPD always enabled. So there we'll miss the
>>> > HPD interrupt anyway, and the EDID cache would be invalid.
>>> >
>>> If omapdss already handles the possibility of display changed during
>>> suspend, I think we should be good :)
>>
>> Hmm I'm not sure I understand what you mean. I was referring to your
>> patch, which invalidated the EDID cache only on HPD interrupt when the
>> cable is unplugged. And we'd miss that interrupt when the board is in
>> system suspend, even if we otherwise kept the HPD interrupt always
>> enabled.
>>
> I meant before stale-edid, we face potential problem of omapdss
> behaving badly to the displays switched during suspend ?
>
OK, I think I get now what you mean. We do need to invalidate
edid-cache in the suspend path, irrespective of how omapdss behaves.
Thanks.
^ permalink raw reply
* Re: [PATCH 2/3] OMAPDSS: HDMI: Replace spinlock with mutex in hdmi_check_hpd_state
From: Tomi Valkeinen @ 2012-06-29 6:45 UTC (permalink / raw)
To: jaswinder.singh
Cc: mythripk, linux-omap, linux-fbdev, andy.green, n-dechesne,
patches
In-Reply-To: <1340805896-28731-1-git-send-email-jaswinder.singh@linaro.org>
[-- Attachment #1: Type: text/plain, Size: 2744 bytes --]
On Wed, 2012-06-27 at 19:34 +0530, jaswinder.singh@linaro.org wrote:
> From: Jassi Brar <jaswinder.singh@linaro.org>
>
> State change of HDMI PHY could potentially take many millisecs, we can do
> better by protecting things in hdmi_set_phy_pwr() with a mutex rather than
> a spin_lock_irqsave.
>
> Signed-off-by: Jassi Brar <jaswinder.singh@linaro.org>
> ---
> drivers/video/omap2/dss/hdmi.c | 1 +
> drivers/video/omap2/dss/ti_hdmi.h | 1 +
> drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c | 6 ++----
> 3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
> index 8195c71..0738090 100644
> --- a/drivers/video/omap2/dss/hdmi.c
> +++ b/drivers/video/omap2/dss/hdmi.c
> @@ -758,6 +758,7 @@ static int __init omapdss_hdmihw_probe(struct platform_device *pdev)
> hdmi.ip_data.core_av_offset = HDMI_CORE_AV;
> hdmi.ip_data.pll_offset = HDMI_PLLCTRL;
> hdmi.ip_data.phy_offset = HDMI_PHY;
> + mutex_init(&hdmi.ip_data.lock);
>
> hdmi_panel_init();
>
> diff --git a/drivers/video/omap2/dss/ti_hdmi.h b/drivers/video/omap2/dss/ti_hdmi.h
> index d174ca1..cc292b8 100644
> --- a/drivers/video/omap2/dss/ti_hdmi.h
> +++ b/drivers/video/omap2/dss/ti_hdmi.h
> @@ -177,6 +177,7 @@ struct hdmi_ip_data {
>
> /* ti_hdmi_4xxx_ip private data. These should be in a separate struct */
> int hpd_gpio;
> + struct mutex lock;
> };
> int ti_hdmi_4xxx_phy_enable(struct hdmi_ip_data *ip_data);
> void ti_hdmi_4xxx_phy_disable(struct hdmi_ip_data *ip_data);
> diff --git a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
> index 3fa3d98..04acca9 100644
> --- a/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
> +++ b/drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c
> @@ -238,10 +238,8 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
> unsigned long flags;
> bool hpd;
> int r;
> - /* this should be in ti_hdmi_4xxx_ip private data */
> - static DEFINE_SPINLOCK(phy_tx_lock);
>
> - spin_lock_irqsave(&phy_tx_lock, flags);
> + mutex_lock(&ip_data->lock);
>
> hpd = gpio_get_value(ip_data->hpd_gpio);
>
> @@ -257,7 +255,7 @@ static int hdmi_check_hpd_state(struct hdmi_ip_data *ip_data)
> }
>
> err:
> - spin_unlock_irqrestore(&phy_tx_lock, flags);
> + mutex_unlock(&ip_data->lock);
> return r;
This introduced a new warning:
drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c: In function 'hdmi_check_hpd_state':
drivers/video/omap2/dss/ti_hdmi_4xxx_ip.c:238:16: warning: unused variable 'flags'
I fixed that.
I'll apply this and the first patch, as I don't think there are any
questions about these.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 07/12] OMAPDSS: APPLY: Remove DISPC writes to manager's lcd parameters in interface drive
From: Tomi Valkeinen @ 2012-06-29 10:12 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-fbdev, linux-omap
In-Reply-To: <1340893842-10626-9-git-send-email-archit@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1701 bytes --]
On Thu, 2012-06-28 at 20:00 +0530, Archit Taneja wrote:
> Replace the DISPC fuctions used to configure LCD channel related manager
> parameters with dss_mgr_set_lcd_config() in APPLY. This function ensures that
> the DISPC registers are written at the right time by using the shadow register
> programming model.
>
> The LCD manager configurations is stored as a private data of manager in APPLY.
> It is treated as an extra info as it's the panel drivers which trigger this
> apply via interface drivers, and not a DSS2 user like omapfb or omapdrm.
>
> Storing LCD manager related properties in APPLY also prevents the need to refer
> to the panel connected to the manager for information. This helps in making the
> DSS driver less dependent on panel.
>
> A helper function is added to check whether the manager is LCD or TV. The direct
> DISPC register writes are removed from the interface drivers.
> +static void dss_apply_mgr_lcd_config(struct omap_overlay_manager *mgr,
> + struct dss_lcd_mgr_config config)
> +{
This one should take a pointer to the config, not a copy (and const).
> + struct mgr_priv_data *mp = get_mgr_priv(mgr);
> +
> + mp->lcd_config = config;
> + mp->extra_info_dirty = true;
> +}
> +
> +void dss_mgr_set_lcd_config(struct omap_overlay_manager *mgr,
> + struct dss_lcd_mgr_config config)
And this.
> +{
> + unsigned long flags;
> + struct mgr_priv_data *mp = get_mgr_priv(mgr);
> +
> + mutex_lock(&apply_lock);
> +
> + if (mp->enabled)
> + goto out;
Hmm. Should we print a warning or such here? Isn't it a bug in the
interface driver, if it tries to set the lcd config when the output is
enabled?
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 07/12] OMAPDSS: APPLY: Remove DISPC writes to manager's lcd parameters in interface drive
From: Archit Taneja @ 2012-06-29 10:40 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-fbdev, linux-omap
In-Reply-To: <1340964745.1866.26.camel@lappyti>
On Friday 29 June 2012 03:42 PM, Tomi Valkeinen wrote:
> On Thu, 2012-06-28 at 20:00 +0530, Archit Taneja wrote:
>> Replace the DISPC fuctions used to configure LCD channel related manager
>> parameters with dss_mgr_set_lcd_config() in APPLY. This function ensures that
>> the DISPC registers are written at the right time by using the shadow register
>> programming model.
>>
>> The LCD manager configurations is stored as a private data of manager in APPLY.
>> It is treated as an extra info as it's the panel drivers which trigger this
>> apply via interface drivers, and not a DSS2 user like omapfb or omapdrm.
>>
>> Storing LCD manager related properties in APPLY also prevents the need to refer
>> to the panel connected to the manager for information. This helps in making the
>> DSS driver less dependent on panel.
>>
>> A helper function is added to check whether the manager is LCD or TV. The direct
>> DISPC register writes are removed from the interface drivers.
>
>
>
>> +static void dss_apply_mgr_lcd_config(struct omap_overlay_manager *mgr,
>> + struct dss_lcd_mgr_config config)
>> +{
>
> This one should take a pointer to the config, not a copy (and const).
>
>> + struct mgr_priv_data *mp = get_mgr_priv(mgr);
>> +
>> + mp->lcd_config = config;
>> + mp->extra_info_dirty = true;
>> +}
>> +
>> +void dss_mgr_set_lcd_config(struct omap_overlay_manager *mgr,
>> + struct dss_lcd_mgr_config config)
>
> And this.
Will fix these.
>
>> +{
>> + unsigned long flags;
>> + struct mgr_priv_data *mp = get_mgr_priv(mgr);
>> +
>> + mutex_lock(&apply_lock);
>> +
>> + if (mp->enabled)
>> + goto out;
>
> Hmm. Should we print a warning or such here? Isn't it a bug in the
> interface driver, if it tries to set the lcd config when the output is
> enabled?
Yes, we should add an error here. Will add it.
Archit
^ permalink raw reply
* [PATCH] pwm-backlight: add regulator and GPIO support
From: Alexandre Courbot @ 2012-06-29 13:22 UTC (permalink / raw)
To: Thierry Reding; +Cc: linux-tegra, linux-kernel, linux-fbdev, Alexandre Courbot
Add support for an optional power regulator and enable/disable GPIO.
This scheme is commonly used in embedded systems.
Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
.../bindings/video/backlight/pwm-backlight | 4 +
drivers/video/backlight/pwm_bl.c | 86 ++++++++++++++++++----
include/linux/pwm_backlight.h | 5 ++
3 files changed, 79 insertions(+), 16 deletions(-)
diff --git a/Documentation/devicetree/bindings/video/backlight/pwm-backlight b/Documentation/devicetree/bindings/video/backlight/pwm-backlight
index 1e4fc72..85cbb7b 100644
--- a/Documentation/devicetree/bindings/video/backlight/pwm-backlight
+++ b/Documentation/devicetree/bindings/video/backlight/pwm-backlight
@@ -14,6 +14,8 @@ Required properties:
Optional properties:
- pwm-names: a list of names for the PWM devices specified in the
"pwms" property (see PWM binding[0])
+ - power-supply: a reference to a regulator used to control the backlight power
+ - enable-gpios: a reference to a GPIO used to enable/disable the backlight
[0]: Documentation/devicetree/bindings/pwm/pwm.txt
@@ -25,4 +27,6 @@ Example:
brightness-levels = <0 4 8 16 32 64 128 255>;
default-brightness-level = <6>;
+ power-supply = <&backlight_reg>;
+ enable-gpios = <&gpio 6 0>;
};
diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
index 057389d..821e03e 100644
--- a/drivers/video/backlight/pwm_bl.c
+++ b/drivers/video/backlight/pwm_bl.c
@@ -20,6 +20,8 @@
#include <linux/pwm.h>
#include <linux/pwm_backlight.h>
#include <linux/slab.h>
+#include <linux/of_gpio.h>
+#include <linux/regulator/consumer.h>
struct pwm_bl_data {
struct pwm_device *pwm;
@@ -27,6 +29,9 @@ struct pwm_bl_data {
unsigned int period;
unsigned int lth_brightness;
unsigned int *levels;
+ bool enabled;
+ struct regulator *power_reg;
+ int enable_gpio;
int (*notify)(struct device *,
int brightness);
void (*notify_after)(struct device *,
@@ -35,6 +40,40 @@ struct pwm_bl_data {
void (*exit)(struct device *);
};
+static void pwm_backlight_off(struct pwm_bl_data *pb)
+{
+ if (!pb->enabled)
+ return;
+
+ if (gpio_is_valid(pb->enable_gpio))
+ gpio_set_value_cansleep(pb->enable_gpio, 0);
+
+ if (pb->power_reg)
+ regulator_disable(pb->power_reg);
+
+ pwm_config(pb->pwm, 0, pb->period);
+ pwm_disable(pb->pwm);
+
+ pb->enabled = false;
+}
+
+static void pwm_backlight_on(struct pwm_bl_data *pb, int brightness)
+{
+ pwm_config(pb->pwm, brightness, pb->period);
+ pwm_enable(pb->pwm);
+
+ if (pb->enabled)
+ return;
+
+ if (pb->power_reg)
+ regulator_enable(pb->power_reg);
+
+ if (gpio_is_valid(pb->enable_gpio))
+ gpio_set_value_cansleep(pb->enable_gpio, 1);
+
+ pb->enabled = true;
+}
+
static int pwm_backlight_update_status(struct backlight_device *bl)
{
struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
@@ -51,8 +90,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
brightness = pb->notify(pb->dev, brightness);
if (brightness = 0) {
- pwm_config(pb->pwm, 0, pb->period);
- pwm_disable(pb->pwm);
+ pwm_backlight_off(pb);
} else {
if (pb->levels) {
brightness = pb->levels[brightness];
@@ -61,8 +99,7 @@ static int pwm_backlight_update_status(struct backlight_device *bl)
brightness = pb->lth_brightness +
(brightness * (pb->period - pb->lth_brightness) / max);
- pwm_config(pb->pwm, brightness, pb->period);
- pwm_enable(pb->pwm);
+ pwm_backlight_on(pb, brightness);
}
if (pb->notify_after)
@@ -141,11 +178,14 @@ static int pwm_backlight_parse_dt(struct device *dev,
data->max_brightness--;
}
- /*
- * TODO: Most users of this driver use a number of GPIOs to control
- * backlight power. Support for specifying these needs to be
- * added.
- */
+ ret = of_get_named_gpio(node, "enable-gpios", 0);
+ if (ret >= 0) {
+ data->enable_gpio = of_get_named_gpio(node, "enable-gpios", 0);
+ data->use_enable_gpio = true;
+ } else if (ret != -ENOENT) {
+ /* GPIO is optional, so ENOENT is not an error here */
+ return ret;
+ }
return 0;
}
@@ -176,7 +216,9 @@ static int pwm_backlight_probe(struct platform_device *pdev)
if (!data) {
ret = pwm_backlight_parse_dt(&pdev->dev, &defdata);
- if (ret < 0) {
+ if (ret = -EPROBE_DEFER) {
+ return ret;
+ } else if (ret < 0) {
dev_err(&pdev->dev, "failed to find platform data\n");
return ret;
}
@@ -221,8 +263,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
}
}
- dev_dbg(&pdev->dev, "got pwm for backlight\n");
-
/*
* The DT case will set the pwm_period_ns field to 0 and store the
* period, parsed from the DT, in the PWM device. For the non-DT case,
@@ -231,6 +271,22 @@ static int pwm_backlight_probe(struct platform_device *pdev)
if (data->pwm_period_ns > 0)
pwm_set_period(pb->pwm, data->pwm_period_ns);
+
+ pb->power_reg = devm_regulator_get(&pdev->dev, "power");
+ if (IS_ERR(pb->power_reg))
+ return PTR_ERR(pb->power_reg);
+
+ pb->enable_gpio = -EINVAL;
+ if (data->use_enable_gpio) {
+ ret = devm_gpio_request_one(&pdev->dev, data->enable_gpio,
+ GPIOF_OUT_INIT_HIGH, "backlight_enable");
+ if (ret)
+ dev_warn(&pdev->dev,
+ "error %d requesting control gpio\n", ret);
+ else
+ pb->enable_gpio = data->enable_gpio;
+ }
+
pb->period = pwm_get_period(pb->pwm);
pb->lth_brightness = data->lth_brightness * (pb->period / max);
@@ -265,8 +321,7 @@ static int pwm_backlight_remove(struct platform_device *pdev)
struct pwm_bl_data *pb = dev_get_drvdata(&bl->dev);
backlight_device_unregister(bl);
- pwm_config(pb->pwm, 0, pb->period);
- pwm_disable(pb->pwm);
+ pwm_backlight_off(pb);
pwm_put(pb->pwm);
if (pb->exit)
pb->exit(&pdev->dev);
@@ -281,8 +336,7 @@ static int pwm_backlight_suspend(struct device *dev)
if (pb->notify)
pb->notify(pb->dev, 0);
- pwm_config(pb->pwm, 0, pb->period);
- pwm_disable(pb->pwm);
+ pwm_backlight_off(pb);
if (pb->notify_after)
pb->notify_after(pb->dev, 0);
return 0;
diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h
index 56f4a86..5ae2cd0 100644
--- a/include/linux/pwm_backlight.h
+++ b/include/linux/pwm_backlight.h
@@ -18,6 +18,11 @@ struct platform_pwm_backlight_data {
void (*notify_after)(struct device *dev, int brightness);
void (*exit)(struct device *dev);
int (*check_fb)(struct device *dev, struct fb_info *info);
+ /* optional GPIO that enables/disables the backlight */
+ int enable_gpio;
+ /* 0 (default initialization value) is a valid GPIO number. Make use of
+ * control gpio explicit to avoid bad surprises. */
+ bool use_enable_gpio;
};
#endif
--
1.7.11.1
^ permalink raw reply related
* Re: [PATCH] pwm-backlight: add regulator and GPIO support
From: Stephen Warren @ 2012-06-29 16:04 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Thierry Reding, linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1340976167-27298-1-git-send-email-acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
On 06/29/2012 07:22 AM, Alexandre Courbot wrote:
> Add support for an optional power regulator and enable/disable GPIO.
> This scheme is commonly used in embedded systems.
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> - dev_dbg(&pdev->dev, "got pwm for backlight\n");
> -
That seems like an unrelated change?
> @@ -231,6 +271,22 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> if (data->pwm_period_ns > 0)
> pwm_set_period(pb->pwm, data->pwm_period_ns);
>
> +
> + pb->power_reg = devm_regulator_get(&pdev->dev, "power");
There's an extra blank line there.
> + if (IS_ERR(pb->power_reg))
> + return PTR_ERR(pb->power_reg);
> +
> + pb->enable_gpio = -EINVAL;
> + if (data->use_enable_gpio) {
> + ret = devm_gpio_request_one(&pdev->dev, data->enable_gpio,
> + GPIOF_OUT_INIT_HIGH, "backlight_enable");
> + if (ret)
> + dev_warn(&pdev->dev,
> + "error %d requesting control gpio\n", ret);
Shouldn't that be a hard error? If the user specified that some GPIO be
used, and the GPIO could not be requested, shouldn't the driver fail to
initialize?
> + else
> + pb->enable_gpio = data->enable_gpio;
Aside from that, this looks good to me.
^ permalink raw reply
* Re: [PATCH] pwm-backlight: add regulator and GPIO support
From: Alex Courbot @ 2012-06-30 3:54 UTC (permalink / raw)
To: Stephen Warren
Cc: Thierry Reding,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <4FEDD222.7050905-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
On 06/30/2012 01:04 AM, Stephen Warren wrote:
>> - dev_dbg(&pdev->dev, "got pwm for backlight\n");
>> -
>
> That seems like an unrelated change?
Dammit, I hoped that would have gone unnoticed. ;)
>> + pb->enable_gpio = -EINVAL;
>> + if (data->use_enable_gpio) {
>> + ret = devm_gpio_request_one(&pdev->dev, data->enable_gpio,
>> + GPIOF_OUT_INIT_HIGH, "backlight_enable");
>> + if (ret)
>> + dev_warn(&pdev->dev,
>> + "error %d requesting control gpio\n", ret);
>
> Shouldn't that be a hard error? If the user specified that some GPIO be
> used, and the GPIO could not be requested, shouldn't the driver fail to
> initialize?
Yes, you are right.
Thanks,
Alex.
^ permalink raw reply
* Re: [PATCH] pwm-backlight: add regulator and GPIO support
From: Thierry Reding @ 2012-06-30 18:37 UTC (permalink / raw)
To: Alexandre Courbot
Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1340976167-27298-1-git-send-email-acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 2342 bytes --]
On Fri, Jun 29, 2012 at 10:22:47PM +0900, Alexandre Courbot wrote:
> Add support for an optional power regulator and enable/disable GPIO.
> This scheme is commonly used in embedded systems.
>
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
I've added some comments in addition to those by Stephen.
[...]
> diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c
> index 057389d..821e03e 100644
> --- a/drivers/video/backlight/pwm_bl.c
> +++ b/drivers/video/backlight/pwm_bl.c
[...]
> @@ -141,11 +178,14 @@ static int pwm_backlight_parse_dt(struct device *dev,
> data->max_brightness--;
> }
>
> - /*
> - * TODO: Most users of this driver use a number of GPIOs to control
> - * backlight power. Support for specifying these needs to be
> - * added.
> - */
> + ret = of_get_named_gpio(node, "enable-gpios", 0);
> + if (ret >= 0) {
> + data->enable_gpio = of_get_named_gpio(node, "enable-gpios", 0);
Can't you just reuse the value of ret here?
[...]
> @@ -231,6 +271,22 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> if (data->pwm_period_ns > 0)
> pwm_set_period(pb->pwm, data->pwm_period_ns);
>
> +
> + pb->power_reg = devm_regulator_get(&pdev->dev, "power");
> + if (IS_ERR(pb->power_reg))
> + return PTR_ERR(pb->power_reg);
> +
> + pb->enable_gpio = -EINVAL;
Perhaps initialize this to -1? Assigning standard error codes to a GPIO
doesn't make much sense.
[...]
> diff --git a/include/linux/pwm_backlight.h b/include/linux/pwm_backlight.h
> index 56f4a86..5ae2cd0 100644
> --- a/include/linux/pwm_backlight.h
> +++ b/include/linux/pwm_backlight.h
> @@ -18,6 +18,11 @@ struct platform_pwm_backlight_data {
> void (*notify_after)(struct device *dev, int brightness);
> void (*exit)(struct device *dev);
> int (*check_fb)(struct device *dev, struct fb_info *info);
> + /* optional GPIO that enables/disables the backlight */
> + int enable_gpio;
> + /* 0 (default initialization value) is a valid GPIO number. Make use of
> + * control gpio explicit to avoid bad surprises. */
> + bool use_enable_gpio;
It's a shame we have to add workarounds like this...
Also the canonical form to write multi-line comments would be:
/*
* 0 (default ...
* ... surprises.
*/
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* [PATCH] s3fb: Add Virge/MX (86C260)
From: Ondrej Zary @ 2012-07-01 19:23 UTC (permalink / raw)
To: Florian Tobias Schandinat
Cc: Ondrej Zajicek, linux-fbdev, Kernel development list
Add support for Virge/MX (86C260) chip. Although this is a laptop chip,
there's an AGP card with this chip too.
Tested with AGP card, will probably not work correctly with laptops.
DDC does not work on this card (even in DOS or Windows).
Signed-off-by: Ondrej Zary <linux@rainbow-software.org>
--- a/drivers/video/s3fb.c
+++ b/drivers/video/s3fb.c
@@ -84,7 +84,7 @@ static const char * const s3_names[] = {"S3 Unknown", "S3 Trio32", "S3 Trio64",
"S3 Virge/VX", "S3 Virge/DX", "S3 Virge/GX",
"S3 Virge/GX2", "S3 Virge/GX2+", "",
"S3 Trio3D/1X", "S3 Trio3D/2X", "S3 Trio3D/2X",
- "S3 Trio3D"};
+ "S3 Trio3D", "S3 Virge/MX"};
#define CHIP_UNKNOWN 0x00
#define CHIP_732_TRIO32 0x01
@@ -105,6 +105,7 @@ static const char * const s3_names[] = {"S3 Unknown", "S3 Trio32", "S3 Trio64",
#define CHIP_362_TRIO3D_2X 0x11
#define CHIP_368_TRIO3D_2X 0x12
#define CHIP_365_TRIO3D 0x13
+#define CHIP_260_VIRGE_MX 0x14
#define CHIP_XXX_TRIO 0x80
#define CHIP_XXX_TRIO64V2_DXGX 0x81
@@ -280,7 +281,8 @@ static int __devinit s3fb_setup_ddc_bus(struct fb_info *info)
*/
/* vga_wseq(par->state.vgabase, 0x08, 0x06); - not needed, already unlocked */
if (par->chip = CHIP_357_VIRGE_GX2 ||
- par->chip = CHIP_359_VIRGE_GX2P)
+ par->chip = CHIP_359_VIRGE_GX2P ||
+ par->chip = CHIP_260_VIRGE_MX)
svga_wseq_mask(par->state.vgabase, 0x0d, 0x01, 0x03);
else
svga_wseq_mask(par->state.vgabase, 0x0d, 0x00, 0x03);
@@ -487,7 +489,8 @@ static void s3_set_pixclock(struct fb_info *info, u32 pixclock)
par->chip = CHIP_359_VIRGE_GX2P ||
par->chip = CHIP_360_TRIO3D_1X ||
par->chip = CHIP_362_TRIO3D_2X ||
- par->chip = CHIP_368_TRIO3D_2X) {
+ par->chip = CHIP_368_TRIO3D_2X ||
+ par->chip = CHIP_260_VIRGE_MX) {
vga_wseq(par->state.vgabase, 0x12, (n - 2) | ((r & 3) << 6)); /* n and two bits of r */
vga_wseq(par->state.vgabase, 0x29, r >> 2); /* remaining highest bit of r */
} else
@@ -690,7 +693,8 @@ static int s3fb_set_par(struct fb_info *info)
par->chip != CHIP_359_VIRGE_GX2P &&
par->chip != CHIP_360_TRIO3D_1X &&
par->chip != CHIP_362_TRIO3D_2X &&
- par->chip != CHIP_368_TRIO3D_2X) {
+ par->chip != CHIP_368_TRIO3D_2X &&
+ par->chip != CHIP_260_VIRGE_MX) {
vga_wcrt(par->state.vgabase, 0x54, 0x18); /* M parameter */
vga_wcrt(par->state.vgabase, 0x60, 0xff); /* N parameter */
vga_wcrt(par->state.vgabase, 0x61, 0xff); /* L parameter */
@@ -739,7 +743,8 @@ static int s3fb_set_par(struct fb_info *info)
par->chip = CHIP_368_TRIO3D_2X ||
par->chip = CHIP_365_TRIO3D ||
par->chip = CHIP_375_VIRGE_DX ||
- par->chip = CHIP_385_VIRGE_GX) {
+ par->chip = CHIP_385_VIRGE_GX ||
+ par->chip = CHIP_260_VIRGE_MX) {
dbytes = info->var.xres * ((bpp+7)/8);
vga_wcrt(par->state.vgabase, 0x91, (dbytes + 7) / 8);
vga_wcrt(par->state.vgabase, 0x90, (((dbytes + 7) / 8) >> 8) | 0x80);
@@ -751,7 +756,8 @@ static int s3fb_set_par(struct fb_info *info)
par->chip = CHIP_359_VIRGE_GX2P ||
par->chip = CHIP_360_TRIO3D_1X ||
par->chip = CHIP_362_TRIO3D_2X ||
- par->chip = CHIP_368_TRIO3D_2X)
+ par->chip = CHIP_368_TRIO3D_2X ||
+ par->chip = CHIP_260_VIRGE_MX)
vga_wcrt(par->state.vgabase, 0x34, 0x00);
else /* enable Data Transfer Position Control (DTPC) */
vga_wcrt(par->state.vgabase, 0x34, 0x10);
@@ -807,7 +813,8 @@ static int s3fb_set_par(struct fb_info *info)
par->chip = CHIP_359_VIRGE_GX2P ||
par->chip = CHIP_360_TRIO3D_1X ||
par->chip = CHIP_362_TRIO3D_2X ||
- par->chip = CHIP_368_TRIO3D_2X)
+ par->chip = CHIP_368_TRIO3D_2X ||
+ par->chip = CHIP_260_VIRGE_MX)
svga_wcrt_mask(par->state.vgabase, 0x67, 0x00, 0xF0);
else {
svga_wcrt_mask(par->state.vgabase, 0x67, 0x10, 0xF0);
@@ -837,7 +844,8 @@ static int s3fb_set_par(struct fb_info *info)
par->chip != CHIP_359_VIRGE_GX2P &&
par->chip != CHIP_360_TRIO3D_1X &&
par->chip != CHIP_362_TRIO3D_2X &&
- par->chip != CHIP_368_TRIO3D_2X)
+ par->chip != CHIP_368_TRIO3D_2X &&
+ par->chip != CHIP_260_VIRGE_MX)
hmul = 2;
}
break;
@@ -864,7 +872,8 @@ static int s3fb_set_par(struct fb_info *info)
par->chip != CHIP_359_VIRGE_GX2P &&
par->chip != CHIP_360_TRIO3D_1X &&
par->chip != CHIP_362_TRIO3D_2X &&
- par->chip != CHIP_368_TRIO3D_2X)
+ par->chip != CHIP_368_TRIO3D_2X &&
+ par->chip != CHIP_260_VIRGE_MX)
hmul = 2;
}
break;
@@ -1208,7 +1217,8 @@ static int __devinit s3_pci_probe(struct pci_dev *dev, const struct pci_device_i
break;
}
} else if (par->chip = CHIP_357_VIRGE_GX2 ||
- par->chip = CHIP_359_VIRGE_GX2P) {
+ par->chip = CHIP_359_VIRGE_GX2P ||
+ par->chip = CHIP_260_VIRGE_MX) {
switch ((regval & 0xC0) >> 6) {
case 1: /* 4MB */
info->screen_size = 4 << 20;
@@ -1515,6 +1525,7 @@ static struct pci_device_id s3_devices[] __devinitdata = {
{PCI_DEVICE(PCI_VENDOR_ID_S3, 0x8A12), .driver_data = CHIP_359_VIRGE_GX2P},
{PCI_DEVICE(PCI_VENDOR_ID_S3, 0x8A13), .driver_data = CHIP_36X_TRIO3D_1X_2X},
{PCI_DEVICE(PCI_VENDOR_ID_S3, 0x8904), .driver_data = CHIP_365_TRIO3D},
+ {PCI_DEVICE(PCI_VENDOR_ID_S3, 0x8C01), .driver_data = CHIP_260_VIRGE_MX},
{0, 0, 0, 0, 0, 0, 0}
};
--
Ondrej Zary
^ permalink raw reply
* Re: [PATCH 3/3] mx3fb: avoid screen flash when panning with fb_set_var
From: Liu Ying @ 2012-07-02 2:18 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1339376810-8247-3-git-send-email-Ying.Liu@freescale.com>
Hi, Guennadi,
Could you please review the second(2/3) patch and the third(3/3) patch
of this series?
I have addressed your comments you gave in the previous review cycle.
Thanks.
2012/6/11, Liu Ying <Ying.Liu@freescale.com>:
> Users may call FBIOPUT_VSCREENINFO ioctrl to do pan display.
> This ioctrl relies on fb_set_var() to do the job. fb_set_var()
> calls custom fb_set_par() method and then calls custom
> fb_pan_display() method. The current implementation of mx3fb
> reinitializes IPU display controller every time the custom
> fb_set_par() method is called, which makes the screen flash
> if fb_set_var() is called to do panning frequently. This patch
> compares the new var info with the cached old one to decide
> whether we really need to reinitialize IPU display controller.
> We ignore xoffset and yoffset update because it doesn't require
> to reinitialize the controller. Users may specify activate field
> of var info with FB_ACTIVATE_NOW and FB_ACTIVATE_FORCE to
> reinialize the controller by force.
>
> Signed-off-by: Liu Ying <Ying.Liu@freescale.com>
> ---
> drivers/video/mx3fb.c | 22 +++++++++++++++++++++-
> 1 files changed, 21 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/video/mx3fb.c b/drivers/video/mx3fb.c
> index 2dd11c4..7d0aa7b 100644
> --- a/drivers/video/mx3fb.c
> +++ b/drivers/video/mx3fb.c
> @@ -721,6 +721,26 @@ static void mx3fb_dma_done(void *arg)
> complete(&mx3_fbi->flip_cmpl);
> }
>
> +static bool mx3fb_must_set_par(struct fb_info *fbi)
> +{
> + struct mx3fb_info *mx3_fbi = fbi->par;
> + struct fb_var_screeninfo old_var = mx3_fbi->cur_var;
> + struct fb_var_screeninfo new_var = fbi->var;
> +
> + if ((fbi->var.activate & FB_ACTIVATE_FORCE) &&
> + (fbi->var.activate & FB_ACTIVATE_MASK) = FB_ACTIVATE_NOW)
> + return true;
> +
> + /*
> + * Ignore xoffset and yoffset update,
> + * because pan display handles this case.
> + */
> + old_var.xoffset = new_var.xoffset;
> + old_var.yoffset = new_var.yoffset;
> +
> + return !!memcmp(&old_var, &new_var, sizeof(struct fb_var_screeninfo));
> +}
> +
> static int __set_par(struct fb_info *fbi, bool lock)
> {
> u32 mem_len, cur_xoffset, cur_yoffset;
> @@ -844,7 +864,7 @@ static int mx3fb_set_par(struct fb_info *fbi)
>
> mutex_lock(&mx3_fbi->mutex);
>
> - ret = __set_par(fbi, true);
> + ret = mx3fb_must_set_par(fbi) ? __set_par(fbi, true) : 0;
>
> mutex_unlock(&mx3_fbi->mutex);
>
> --
> 1.7.1
>
>
>
--
Best Regards,
Liu Ying
^ permalink raw reply
* Re: [PATCH] pwm-backlight: add regulator and GPIO support
From: Alexandre Courbot @ 2012-07-02 3:35 UTC (permalink / raw)
To: Thierry Reding, Stephen Warren
Cc: linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <20120630183742.GE23990-RM9K5IK7kjIyiCvfTdI0JKcOhU4Rzj621B7CTYaBSLdn68oJJulU0Q@public.gmane.org>
On 07/01/2012 03:37 AM, Thierry Reding wrote:>> + ret =
of_get_named_gpio(node, "enable-gpios", 0);
>> + if (ret >= 0) {
>> + data->enable_gpio = of_get_named_gpio(node, "enable-gpios", 0);
>
> Can't you just reuse the value of ret here?
Yes, definitely.
>> + pb->enable_gpio = -EINVAL;
>
> Perhaps initialize this to -1? Assigning standard error codes to a GPIO
> doesn't make much sense.
Documentation/gpio.txt states the following:
"If you want to initialize a structure with an invalid GPIO number, use
some negative number (perhaps "-EINVAL"); that will never be valid."
gpio_is_valid() seems to be happy with any negative value, but -EINVAL
seems to be a convention here.
>> + /* optional GPIO that enables/disables the backlight */
>> + int enable_gpio;
>> + /* 0 (default initialization value) is a valid GPIO number. Make
use of
>> + * control gpio explicit to avoid bad surprises. */
>> + bool use_enable_gpio;
>
> It's a shame we have to add workarounds like this...
Yeah, I hate that too. :/ I see nothing better to do unfortunately.
Other remarks from Stephen made me realize that this patch has two major
flaws:
1) The GPIO and regulator are fixed, optional entites ; this should
cover most cases but is not very flexible.
2) Some (most?) backlight specify timings between turning power
on/enabling PWM/enabling backlight. Even the order of things may be
different. This patch totally ignores that.
So instead of having fixed "power-supply" and "enable-gpio" properties,
how about having properties describing the power-on and power-off
sequences which odd cells alternate between phandles to
regulators/gpios/pwm and delays in microseconds before continuing the
sequence. For example:
power-on = <&pwm 2 5000000
10000
&backlight_reg
0
&gpio 28 0>;
power-off = <&gpio 28 0
0
&backlight_reg
10000
&pwm 2 0>;
Here the power-on sequence would translate to, power on the second PWM
with a duty-cycle of 5ms, wait 10ms, then enable the backlight regulator
and GPIO 28 without delay. Power-off is the exact opposite. The nice
thing with this scheme is that you can reorder the sequence at will and
support the weirdest setups.
What I don't know (device tree newbie here!) is:
1) Is it legal to refer the same phandle several times in the same node?
2) Is it ok to mix phandles of different types with integer values? The
DT above compiled, but can you e.g. resolve a regulator phandle in the
middle of a property?
3) Can you "guess" the type of a phandle before parsing it? Here the
first phandle is a GPIO, but it could as well be the regulator. Do we
have means to know that in the driver code?
Sorry for the long explanation, but I really wonder if doing this is
possible at all. If it is, then I think that's the way to do for
backlight initialization.
Alex.
^ permalink raw reply
* Re: [PATCH 2/3] mx3fb: support pan display with fb_set_var
From: Florian Tobias Schandinat @ 2012-07-02 6:28 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1339376810-8247-2-git-send-email-Ying.Liu@freescale.com>
On 06/11/2012 01:06 AM, Liu Ying wrote:
> Users may call FBIOPUT_VSCREENINFO ioctrl to do pan display.
> This ioctrl relies on fb_set_var() to do the job. fb_set_var()
> calls the custom fb_set_par() method and then calls the custom
> fb_pan_display() method. Before calling the custom fb_pan_display()
> method, info->var is already updated from the new *var in fb_set_var().
> And, the custom fb_pan_display() method checks if xoffset and yoffset
> in info->var and the new *var are different before doing actual panning,
> which prevents the panning from happening within fb_set_var() context.
> This patch caches the current var info locally in mx3fb driver so that
> pan display with fb_set_var is supported.
>
> Signed-off-by: Liu Ying <Ying.Liu@freescale.com>
Applied patches 2 and 3 of this series.
Thanks,
Florian Tobias Schandinat
> ---
> drivers/video/mx3fb.c | 33 ++++++++++++++++++++++++++-------
> 1 files changed, 26 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/video/mx3fb.c b/drivers/video/mx3fb.c
> index d53db60..2dd11c4 100644
> --- a/drivers/video/mx3fb.c
> +++ b/drivers/video/mx3fb.c
> @@ -268,7 +268,7 @@ struct mx3fb_info {
> dma_cookie_t cookie;
> struct scatterlist sg[2];
>
> - u32 sync; /* preserve var->sync flags */
> + struct fb_var_screeninfo cur_var; /* current var info */
> };
>
> static void mx3fb_dma_done(void *);
> @@ -723,7 +723,7 @@ static void mx3fb_dma_done(void *arg)
>
> static int __set_par(struct fb_info *fbi, bool lock)
> {
> - u32 mem_len;
> + u32 mem_len, cur_xoffset, cur_yoffset;
> struct ipu_di_signal_cfg sig_cfg;
> enum ipu_panel mode = IPU_PANEL_TFT;
> struct mx3fb_info *mx3_fbi = fbi->par;
> @@ -805,8 +805,25 @@ static int __set_par(struct fb_info *fbi, bool lock)
> video->out_height = fbi->var.yres;
> video->out_stride = fbi->var.xres_virtual;
>
> - if (mx3_fbi->blank = FB_BLANK_UNBLANK)
> + if (mx3_fbi->blank = FB_BLANK_UNBLANK) {
> sdc_enable_channel(mx3_fbi);
> + /*
> + * sg[0] points to fb smem_start address
> + * and is actually active in controller.
> + */
> + mx3_fbi->cur_var.xoffset = 0;
> + mx3_fbi->cur_var.yoffset = 0;
> + }
> +
> + /*
> + * Preserve xoffset and yoffest in case they are
> + * inactive in controller as fb is blanked.
> + */
> + cur_xoffset = mx3_fbi->cur_var.xoffset;
> + cur_yoffset = mx3_fbi->cur_var.yoffset;
> + mx3_fbi->cur_var = fbi->var;
> + mx3_fbi->cur_var.xoffset = cur_xoffset;
> + mx3_fbi->cur_var.yoffset = cur_yoffset;
>
> return 0;
> }
> @@ -926,8 +943,8 @@ static int mx3fb_check_var(struct fb_var_screeninfo *var, struct fb_info *fbi)
> var->grayscale = 0;
>
> /* Preserve sync flags */
> - var->sync |= mx3_fbi->sync;
> - mx3_fbi->sync |= var->sync;
> + var->sync |= mx3_fbi->cur_var.sync;
> + mx3_fbi->cur_var.sync |= var->sync;
>
> return 0;
> }
> @@ -1073,8 +1090,8 @@ static int mx3fb_pan_display(struct fb_var_screeninfo *var,
> return -EINVAL;
> }
>
> - if (fbi->var.xoffset = var->xoffset &&
> - fbi->var.yoffset = var->yoffset)
> + if (mx3_fbi->cur_var.xoffset = var->xoffset &&
> + mx3_fbi->cur_var.yoffset = var->yoffset)
> return 0; /* No change, do nothing */
>
> y_bottom = var->yoffset;
> @@ -1157,6 +1174,8 @@ static int mx3fb_pan_display(struct fb_var_screeninfo *var,
> else
> fbi->var.vmode &= ~FB_VMODE_YWRAP;
>
> + mx3_fbi->cur_var = fbi->var;
> +
> mutex_unlock(&mx3_fbi->mutex);
>
> dev_dbg(fbi->device, "Update complete\n");
^ permalink raw reply
* Re: [PATCH] da8xx-fb: Rounding FB size to satisfy SGX buffer requirements
From: Florian Tobias Schandinat @ 2012-07-02 6:37 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1337753191-14802-1-git-send-email-aditya.n@ti.com>
On 06/12/2012 09:34 AM, Nellutla, Aditya wrote:
> Florian,
>
> I didn't receive any review comments for the patch below. So could you please merge the patch?
As far as I can see there is no feasible alternative so I applied this
patch. If anyone happens to have problems with the larger memory
requirements caused by this, I guess we could make this behavior depend
on a Kconfig symbol.
Best regards,
Florian Tobias Schandinat
>
> Regards,
> Aditya
>
> -----Original Message-----
> From: Nellutla, Aditya
> Sent: Monday, June 04, 2012 11:33 AM
> To: Nellutla, Aditya; linux-fbdev@vger.kernel.org
> Cc: Hiremath, Vaibhav
> Subject: RE: [PATCH] da8xx-fb: Rounding FB size to satisfy SGX buffer requirements
>
> All,
> Did you get chance to review this patch? Request to give your feedback as early as possible.
>
> Regards,
> Aditya
>
> -----Original Message-----
> From: Nellutla, Aditya
> Sent: Wednesday, May 23, 2012 11:37 AM
> To: linux-fbdev@vger.kernel.org
> Cc: Nellutla, Aditya
> Subject: [PATCH] da8xx-fb: Rounding FB size to satisfy SGX buffer requirements
>
> In the real time use-case when SGX is used for rendering to FB buffers it has been
> observed that, the available memory from framebuffer driver is not sufficient for
> SGX under certain cases (like 16-bit WVGA resolution). SGX requires 2 swap buffers
> with each of the buffers aligned to lcm(line_length, PAGE_SIZE).
>
> Inorder to satisfy this requirement, we have two options,
>
> - Increase number of FB buffers (LCD_NUM_BUFFERS) to 3. This is not
> recommended as we end up wasting huge memory in most of the cases.
>
> - Align FB buffers to lcm(line_length, PAGE_SIZE).This ensures framebuffer
> size is increased to satisfy SGX requirements keeping alignment intact.
>
> This patch makes sure that FB allocates buffers aligned to above formula.
>
> Signed-off-by: Aditya Nellutla <aditya.n@ti.com>
> ---
> drivers/video/da8xx-fb.c | 5 ++++-
> 1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
> index 47118c7..2f24c19 100644
> --- a/drivers/video/da8xx-fb.c
> +++ b/drivers/video/da8xx-fb.c
> @@ -31,6 +31,7 @@
> #include <linux/cpufreq.h>
> #include <linux/console.h>
> #include <linux/slab.h>
> +#include <linux/lcm.h>
> #include <video/da8xx-fb.h>
> #include <asm/div64.h>
>
> @@ -1114,6 +1115,7 @@ static int __devinit fb_probe(struct platform_device *device)
> struct da8xx_fb_par *par;
> resource_size_t len;
> int ret, i;
> + unsigned long ulcm;
>
> if (fb_pdata = NULL) {
> dev_err(&device->dev, "Can not get platform data\n");
> @@ -1209,7 +1211,8 @@ static int __devinit fb_probe(struct platform_device *device)
>
> /* allocate frame buffer */
> par->vram_size = lcdc_info->width * lcdc_info->height * lcd_cfg->bpp;
> - par->vram_size = PAGE_ALIGN(par->vram_size/8);
> + ulcm = lcm((lcdc_info->width * lcd_cfg->bpp)/8, PAGE_SIZE);
> + par->vram_size = roundup(par->vram_size/8, ulcm);
> par->vram_size = par->vram_size * LCD_NUM_BUFFERS;
>
> par->vram_virt = dma_alloc_coherent(NULL,
^ permalink raw reply
* Re: [PATCH] pwm-backlight: add regulator and GPIO support
From: Thierry Reding @ 2012-07-02 6:46 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Stephen Warren,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ, Mitch Bradley
In-Reply-To: <4FF116F0.5070602-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
[-- Attachment #1: Type: text/plain, Size: 3872 bytes --]
On Mon, Jul 02, 2012 at 12:35:12PM +0900, Alexandre Courbot wrote:
> On 07/01/2012 03:37 AM, Thierry Reding wrote:>> + ret =
> of_get_named_gpio(node, "enable-gpios", 0);
> >> + if (ret >= 0) {
> >> + data->enable_gpio = of_get_named_gpio(node, "enable-gpios", 0);
> >
> > Can't you just reuse the value of ret here?
>
> Yes, definitely.
>
> >> + pb->enable_gpio = -EINVAL;
> >
> > Perhaps initialize this to -1? Assigning standard error codes to a GPIO
> > doesn't make much sense.
>
> Documentation/gpio.txt states the following:
>
> "If you want to initialize a structure with an invalid GPIO number, use
> some negative number (perhaps "-EINVAL"); that will never be valid."
>
> gpio_is_valid() seems to be happy with any negative value, but
> -EINVAL seems to be a convention here.
I would have thought something like -1 would be good enough to represent
an invalid GPIO, but if there's already a convention then we should
follow it.
> >> + /* optional GPIO that enables/disables the backlight */
> >> + int enable_gpio;
> >> + /* 0 (default initialization value) is a valid GPIO number.
> Make use of
> >> + * control gpio explicit to avoid bad surprises. */
> >> + bool use_enable_gpio;
> >
> > It's a shame we have to add workarounds like this...
>
> Yeah, I hate that too. :/ I see nothing better to do unfortunately.
>
> Other remarks from Stephen made me realize that this patch has two
> major flaws:
>
> 1) The GPIO and regulator are fixed, optional entites ; this should
> cover most cases but is not very flexible.
> 2) Some (most?) backlight specify timings between turning power
> on/enabling PWM/enabling backlight. Even the order of things may be
> different. This patch totally ignores that.
>
> So instead of having fixed "power-supply" and "enable-gpio"
> properties, how about having properties describing the power-on and
> power-off sequences which odd cells alternate between phandles to
> regulators/gpios/pwm and delays in microseconds before continuing
> the sequence. For example:
>
> power-on = <&pwm 2 5000000
> 10000
> &backlight_reg
> 0
> &gpio 28 0>;
> power-off = <&gpio 28 0
> 0
> &backlight_reg
> 10000
> &pwm 2 0>;
>
> Here the power-on sequence would translate to, power on the second
> PWM with a duty-cycle of 5ms, wait 10ms, then enable the backlight
> regulator and GPIO 28 without delay. Power-off is the exact
> opposite. The nice thing with this scheme is that you can reorder
> the sequence at will and support the weirdest setups.
I generally like the idea. I'm Cc'ing the devicetree-discuss mailing
list, let's see what people there think about it. I've also added Mitch
Bradley who already helped a lot with the initial binding.
> What I don't know (device tree newbie here!) is:
> 1) Is it legal to refer the same phandle several times in the same node?
> 2) Is it ok to mix phandles of different types with integer values?
> The DT above compiled, but can you e.g. resolve a regulator phandle
> in the middle of a property?
I believe these shouldn't be a problem.
> 3) Can you "guess" the type of a phandle before parsing it? Here the
> first phandle is a GPIO, but it could as well be the regulator. Do
> we have means to know that in the driver code?
That isn't possible. But you could for instance use a cell to specify
the type of the following phandle.
> Sorry for the long explanation, but I really wonder if doing this is
> possible at all. If it is, then I think that's the way to do for
> backlight initialization.
OTOH we basically end up describing a code sequence in the DT. But as
you mention above sometimes the hardware requires specific timing
parameters so this might actually be a kind of valid sequence to
describe in the DT.
Thierry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH] pwm-backlight: add regulator and GPIO support
From: Alexandre Courbot @ 2012-07-02 7:18 UTC (permalink / raw)
To: Thierry Reding
Cc: Stephen Warren,
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,
Mitch Bradley
In-Reply-To: <20120702064624.GA8683-RM9K5IK7kjIQXX3q8xo1gnVAuStQJXxyR5q1nwbD4aMs9pC9oP6+/A@public.gmane.org>
On 07/02/2012 03:46 PM, Thierry Reding wrote:
>> So instead of having fixed "power-supply" and "enable-gpio"
>> properties, how about having properties describing the power-on and
>> power-off sequences which odd cells alternate between phandles to
>> regulators/gpios/pwm and delays in microseconds before continuing
>> the sequence. For example:
>>
>> power-on = <&pwm 2 5000000
>> 10000
>> &backlight_reg
>> 0
>> &gpio 28 0>;
>> power-off = <&gpio 28 0
>> 0
>> &backlight_reg
>> 10000
>> &pwm 2 0>;
>>
>> Here the power-on sequence would translate to, power on the second
>> PWM with a duty-cycle of 5ms, wait 10ms, then enable the backlight
>> regulator and GPIO 28 without delay. Power-off is the exact
>> opposite. The nice thing with this scheme is that you can reorder
>> the sequence at will and support the weirdest setups.
>
> I generally like the idea. I'm Cc'ing the devicetree-discuss mailing
> list, let's see what people there think about it. I've also added Mitch
> Bradley who already helped a lot with the initial binding.
Cool, thanks. I am aware that this idea probably needs to be refined,
but ideally we would end up with something as simple as this example.
>> What I don't know (device tree newbie here!) is:
>> 1) Is it legal to refer the same phandle several times in the same node?
>> 2) Is it ok to mix phandles of different types with integer values?
>> The DT above compiled, but can you e.g. resolve a regulator phandle
>> in the middle of a property?
>
> I believe these shouldn't be a problem.
Nice, I'll try to see how I can parse it then.
>> 3) Can you "guess" the type of a phandle before parsing it? Here the
>> first phandle is a GPIO, but it could as well be the regulator. Do
>> we have means to know that in the driver code?
>
> That isn't possible. But you could for instance use a cell to specify
> the type of the following phandle.
That sounds reasonnable, although we would also need to bind values to
phandle types. That would make the DT a little bit more cryptic,
although nothing too bad I think.
>> Sorry for the long explanation, but I really wonder if doing this is
>> possible at all. If it is, then I think that's the way to do for
>> backlight initialization.
>
> OTOH we basically end up describing a code sequence in the DT. But as
> you mention above sometimes the hardware requires specific timing
> parameters so this might actually be a kind of valid sequence to
> describe in the DT.
Yes, the power on/power off sequences are part of the board/panel
specification - they are a hardware description and thus fits within the
purpose of the device tree IMHO. One could argue that initialization
sequences are usually coded inside drivers, but that only works when the
driver has a single initialization sequence to handle. With
pwm-backlight we try to handle something much more general, and so far
these sequences were performed by board-specific functions.
All in all, adding timings & ordering is not so different from the
original patch which accepted one optional regulator and GPIO.
Also, if someone knows of anything else besides PWM/GPIO/regulators that
may need to be controlled by a backlight power sequence, please let me know.
Alex.
^ permalink raw reply
* RE: [PATCH] da8xx-fb: add missing FB_BLANK operations
From: Hiremath, Vaibhav @ 2012-07-02 7:51 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1341213073-2986-1-git-send-email-yegorslists@googlemail.com>
On Mon, Jul 02, 2012 at 12:41:13, yegorslists@googlemail.com wrote:
> From: Yegor Yefremov <yegorslists@googlemail.com>
>
> add FB_BLANK_NORMAL, FB_BLANK_VSYNC_SUSPEND and FB_BLANK_HSYNC_SUSPEND
> modes (copy drivers/video/omap2/omapfb/omapfb-main.c implementation).
> Otherwise X-server will complain about invalid parameter.
>
> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
> ---
> drivers/video/da8xx-fb.c | 3 +++
> 1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
> index 29577bf..9879edc 100644
> --- a/drivers/video/da8xx-fb.c
> +++ b/drivers/video/da8xx-fb.c
> @@ -987,6 +987,9 @@ static int cfb_blank(int blank, struct fb_info *info)
>
> lcd_enable_raster();
> break;
> + case FB_BLANK_NORMAL:
> + case FB_BLANK_VSYNC_SUSPEND:
> + case FB_BLANK_HSYNC_SUSPEND:
> case FB_BLANK_POWERDOWN:
> if (par->panel_power_ctrl)
> par->panel_power_ctrl(0);
+ linux-fbdev & Florian
You should always send Fbdev patches to linux-fbdev list.
Thanks,
Vaibhav
^ permalink raw reply
* Re: [PATCH] video/smscufx: fix line counting in fb_write
From: Alexander Holler @ 2012-07-02 21:25 UTC (permalink / raw)
To: Florian Tobias Schandinat; +Cc: linux-kernel, linux-fbdev
In-Reply-To: <4FAFAD7B.6090207@gmx.de>
Hi Florian,
sorry for the late answer.
Am 13.05.2012 14:47, schrieb Florian Tobias Schandinat:
> Hi Alexander,
>
> On 04/21/2012 11:26 AM, Alexander Holler wrote:
>> Hello,
>>
>> as for the patch for udlfb, I forgot to mention that this is a candidate
>> for all stable trees 3.2 and above.
>>
>> Btw., the address of the maintainer doesn't seem to be valid anymore.
>
> it is better to cc me on patches to the framebuffer subsystem for such
> cases. I don't have much free time so it's rare that I come around to
> dig in the mailing list.
>
>>
>> Regards,
>>
>> Alexander
>>
>> Am 21.04.2012 00:11, schrieb Alexander Holler:
>>> Line 0 and 1 were both written to line 0 (on the display) and all
>>> subsequent
>>> lines had an offset of -1. The result was that the last line on the
>>> display
>>> was never overwritten by writes to /dev/fbN.
>>>
>>> The origin of this bug seems to have been udlfb.
>>>
>>> Signed-off-by: Alexander Holler<holler@ahsoftware.de>
>>
>> Cc: stable@vger.kernel.org
>
> Patch looks good to me but can be made simpler.
>
>>
>>> ---
>>> drivers/video/smscufx.c | 2 +-
>>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/drivers/video/smscufx.c b/drivers/video/smscufx.c
>>> index ccbfef5..1e1e2d2 100644
>>> --- a/drivers/video/smscufx.c
>>> +++ b/drivers/video/smscufx.c
>>> @@ -904,7 +904,7 @@ static ssize_t ufx_ops_write(struct fb_info *info,
>>> const char __user *buf,
>>> result = fb_sys_write(info, buf, count, ppos);
>>>
>>> if (result> 0) {
>>> - int start = max((int)(offset / info->fix.line_length) - 1, 0);
>>> + int start = max((int)(offset / info->fix.line_length), 0);
>
> the cast to int as well as the max is superfluous without the -1 as the
> value can no longer be negative.
I had that impression too, but I wanted to change as less as possible,
so I didn't had the need to check types and (their) sizes for possible
overflows or such. I was lazy and just wanted to fix that one bug. ;)
>
>>> int lines = min((u32)((result / info->fix.line_length) + 1),
>>> (u32)info->var.yres);
>>>
Regards,
Alexander
^ permalink raw reply
* Re: [PATCH 0/2] simple device tree support for mxsfb
From: Shawn Guo @ 2012-07-03 7:46 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <1340701379-8619-1-git-send-email-shawn.guo@linaro.org>
Hi Florian,
On Tue, Jun 26, 2012 at 05:02:57PM +0800, Shawn Guo wrote:
> It adds a simple device tree support for mxsfb driver.
>
> Shawn Guo (2):
> video: mxsfb: move mxsfb.h into include/linux
> video: mxsfb: add simple device tree probe
>
I just queued these two patches on mxs arch tree. Please let me know
if you see problem with that.
Regards,
Shawn
> Documentation/devicetree/bindings/fb/mxsfb.txt | 21 +++++++
> arch/arm/mach-mxs/devices-mx23.h | 2 +-
> arch/arm/mach-mxs/devices-mx28.h | 2 +-
> arch/arm/mach-mxs/devices/platform-mxsfb.c | 2 +-
> drivers/video/mxsfb.c | 62 +++++++++++++++-----
> .../include/mach => include/linux}/mxsfb.h | 6 +-
> 6 files changed, 75 insertions(+), 20 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/fb/mxsfb.txt
> rename {arch/arm/mach-mxs/include/mach => include/linux}/mxsfb.h (95%)
>
> --
> 1.7.5.4
>
>
^ permalink raw reply
* [PATCH v2] of: Add videomode helper
From: Sascha Hauer @ 2012-07-04 7:56 UTC (permalink / raw)
To: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA, Sascha Hauer,
dri-devel-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW, Laurent Pinchart,
kernel-bIcnvbaLZ9MEGnE8C9+IrQ
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.
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
changes since v1:
- use hyphens instead of underscores for property names
.../devicetree/bindings/video/displaymode | 40 ++++++++
drivers/of/Kconfig | 5 +
drivers/of/Makefile | 1 +
drivers/of/of_videomode.c | 108 ++++++++++++++++++++
include/linux/of_videomode.h | 19 ++++
5 files changed, 173 insertions(+)
create mode 100644 Documentation/devicetree/bindings/video/displaymode
create mode 100644 drivers/of/of_videomode.c
create mode 100644 include/linux/of_videomode.h
diff --git a/Documentation/devicetree/bindings/video/displaymode b/Documentation/devicetree/bindings/video/displaymode
new file mode 100644
index 0000000..43cc17d
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/displaymode
@@ -0,0 +1,40 @@
+videomode bindings
+=========
+
+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
+ - 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.
+
+Example:
+
+ display@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;
+ };
+
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index dfba3e6..a3acaa3 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -83,4 +83,9 @@ config OF_MTD
depends on MTD
def_bool y
+config OF_VIDEOMODE
+ def_bool y
+ help
+ helper to parse videomodes from the devicetree
+
endmenu # OF
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index e027f44..80e6db3 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -11,3 +11,4 @@ obj-$(CONFIG_OF_MDIO) += of_mdio.o
obj-$(CONFIG_OF_PCI) += of_pci.o
obj-$(CONFIG_OF_PCI_IRQ) += of_pci_irq.o
obj-$(CONFIG_OF_MTD) += of_mtd.o
+obj-$(CONFIG_OF_VIDEOMODE) += of_videomode.o
diff --git a/drivers/of/of_videomode.c b/drivers/of/of_videomode.c
new file mode 100644
index 0000000..50d3bd2
--- /dev/null
+++ b/drivers/of/of_videomode.c
@@ -0,0 +1,108 @@
+/*
+ * OF helpers for parsing display modes
+ *
+ * Copyright (c) 2012 Sascha Hauer <s.hauer@pengutronix.de>, Pengutronix
+ *
+ * This file is released under the GPLv2
+ */
+#include <linux/of.h>
+#include <linux/fb.h>
+#include <linux/export.h>
+#include <drm/drmP.h>
+#include <drm/drm_crtc.h>
+
+int of_get_video_mode(struct device_node *np, struct drm_display_mode *dmode,
+ struct fb_videomode *fbmode)
+{
+ int ret = 0;
+ u32 left_margin, xres, right_margin, hsync_len;
+ u32 upper_margin, yres, lower_margin, vsync_len;
+ u32 width_mm = 0, height_mm = 0;
+ u32 clock;
+ bool hah = false, vah = false, interlaced = false, doublescan = false;
+
+ if (!np)
+ return -EINVAL;
+
+ ret |= of_property_read_u32(np, "left-margin", &left_margin);
+ ret |= of_property_read_u32(np, "xres", &xres);
+ ret |= of_property_read_u32(np, "right-margin", &right_margin);
+ ret |= of_property_read_u32(np, "hsync-len", &hsync_len);
+ ret |= of_property_read_u32(np, "upper-margin", &upper_margin);
+ ret |= of_property_read_u32(np, "yres", &yres);
+ ret |= of_property_read_u32(np, "lower-margin", &lower_margin);
+ ret |= of_property_read_u32(np, "vsync-len", &vsync_len);
+ ret |= of_property_read_u32(np, "clock", &clock);
+ if (ret)
+ return -EINVAL;
+
+ of_property_read_u32(np, "width-mm", &width_mm);
+ of_property_read_u32(np, "height-mm", &height_mm);
+
+ hah = of_property_read_bool(np, "hsync-active-high");
+ vah = of_property_read_bool(np, "vsync-active-high");
+ interlaced = of_property_read_bool(np, "interlaced");
+ doublescan = of_property_read_bool(np, "doublescan");
+
+ if (dmode) {
+ memset(dmode, 0, sizeof(*dmode));
+
+ dmode->hdisplay = xres;
+ dmode->hsync_start = xres + right_margin;
+ dmode->hsync_end = xres + right_margin + hsync_len;
+ dmode->htotal = xres + right_margin + hsync_len + left_margin;
+
+ dmode->vdisplay = yres;
+ dmode->vsync_start = yres + lower_margin;
+ dmode->vsync_end = yres + lower_margin + vsync_len;
+ dmode->vtotal = yres + lower_margin + vsync_len + upper_margin;
+
+ dmode->width_mm = width_mm;
+ dmode->height_mm = height_mm;
+
+ dmode->clock = clock / 1000;
+
+ if (hah)
+ dmode->flags |= DRM_MODE_FLAG_PHSYNC;
+ else
+ dmode->flags |= DRM_MODE_FLAG_NHSYNC;
+ if (vah)
+ dmode->flags |= DRM_MODE_FLAG_PVSYNC;
+ else
+ dmode->flags |= DRM_MODE_FLAG_NVSYNC;
+ if (interlaced)
+ dmode->flags |= DRM_MODE_FLAG_INTERLACE;
+ if (doublescan)
+ dmode->flags |= DRM_MODE_FLAG_DBLSCAN;
+
+ drm_mode_set_name(dmode);
+ }
+
+ if (fbmode) {
+ memset(fbmode, 0, sizeof(*fbmode));
+
+ fbmode->xres = xres;
+ fbmode->left_margin = left_margin;
+ fbmode->right_margin = right_margin;
+ fbmode->hsync_len = hsync_len;
+
+ fbmode->yres = yres;
+ fbmode->upper_margin = upper_margin;
+ fbmode->lower_margin = lower_margin;
+ fbmode->vsync_len = vsync_len;
+
+ fbmode->pixclock = KHZ2PICOS(clock / 1000);
+
+ if (hah)
+ fbmode->sync |= FB_SYNC_HOR_HIGH_ACT;
+ if (vah)
+ fbmode->sync |= FB_SYNC_VERT_HIGH_ACT;
+ if (interlaced)
+ fbmode->vmode |= FB_VMODE_INTERLACED;
+ if (doublescan)
+ fbmode->vmode |= FB_VMODE_DOUBLE;
+ }
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(of_get_video_mode);
diff --git a/include/linux/of_videomode.h b/include/linux/of_videomode.h
new file mode 100644
index 0000000..a988429
--- /dev/null
+++ b/include/linux/of_videomode.h
@@ -0,0 +1,19 @@
+/*
+ * Copyright 2012 Sascha Hauer <s.hauer@pengutronix.de>
+ *
+ * OF helpers for videomodes.
+ *
+ * This file is released under the GPLv2
+ */
+
+#ifndef __LINUX_OF_VIDEOMODE_H
+#define __LINUX_OF_VIDEOMODE_H
+
+struct device_node;
+struct fb_videomode;
+struct drm_display_mode;
+
+int of_get_video_mode(struct device_node *np, struct drm_display_mode *dmode,
+ struct fb_videomode *fbmode);
+
+#endif /* __LINUX_OF_VIDEOMODE_H */
--
1.7.10
^ permalink raw reply related
* Re: [PATCH] pwm-backlight: add regulator and GPIO support
From: Sascha Hauer @ 2012-07-04 10:48 UTC (permalink / raw)
To: Alexandre Courbot
Cc: Thierry Reding, linux-tegra-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1340976167-27298-1-git-send-email-acourbot-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
Hi Alexandre,
On Fri, Jun 29, 2012 at 10:22:47PM +0900, Alexandre Courbot wrote:
> @@ -221,8 +263,6 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> }
> }
>
> - dev_dbg(&pdev->dev, "got pwm for backlight\n");
> -
> /*
> * The DT case will set the pwm_period_ns field to 0 and store the
> * period, parsed from the DT, in the PWM device. For the non-DT case,
> @@ -231,6 +271,22 @@ static int pwm_backlight_probe(struct platform_device *pdev)
> if (data->pwm_period_ns > 0)
> pwm_set_period(pb->pwm, data->pwm_period_ns);
>
> +
> + pb->power_reg = devm_regulator_get(&pdev->dev, "power");
> + if (IS_ERR(pb->power_reg))
> + return PTR_ERR(pb->power_reg);
This looses several resources allocated earlier, like the enable gpio
and the pwm. This is really bad here since I have no regulator specified
and devm_regulator_get returns with -EPROBE_DEFER. Next time the core
tries to probe the driver the driver bails out because the gpio and the
pwm is already requested.
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
* Re: [PATCH] pwm-backlight: add regulator and GPIO support
From: Alex Courbot @ 2012-07-04 12:26 UTC (permalink / raw)
To: Sascha Hauer
Cc: Thierry Reding,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <20120704104840.GJ24458-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Hi Sascha,
On 07/04/2012 07:48 PM, Sascha Hauer wrote:>> +
>> + pb->power_reg = devm_regulator_get(&pdev->dev, "power");
>> + if (IS_ERR(pb->power_reg))
>> + return PTR_ERR(pb->power_reg);
>
> This looses several resources allocated earlier, like the enable gpio
> and the pwm. This is really bad here since I have no regulator specified
> and devm_regulator_get returns with -EPROBE_DEFER. Next time the core
> tries to probe the driver the driver bails out because the gpio and the
> pwm is already requested.
That's very bad indeed. I assumed that the kernel would free
devm-allocated resources after probe returned -EPROBE_DEFER, so that
probe could reallocate them the next time it is called. Apparently that
was wrong. Do you know what would the right approach be in this case?
Does the kernel preserve the device structure and its associated data
between the two calls to probe? If so, I could just check whether the
private data has already been constructed to know which state we are in
and continue from there.
Thanks,
Alex.
^ permalink raw reply
* Re: [PATCH] pwm-backlight: add regulator and GPIO support
From: Mark Brown @ 2012-07-04 12:27 UTC (permalink / raw)
To: Alex Courbot
Cc: Sascha Hauer, Thierry Reding,
linux-tegra-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-fbdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <4FF43692.2040805-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
On Wed, Jul 04, 2012 at 09:26:58PM +0900, Alex Courbot wrote:
> On 07/04/2012 07:48 PM, Sascha Hauer wrote:>> +
> > This looses several resources allocated earlier, like the enable gpio
> > and the pwm. This is really bad here since I have no regulator specified
> > and devm_regulator_get returns with -EPROBE_DEFER. Next time the core
> > tries to probe the driver the driver bails out because the gpio and the
> > pwm is already requested.
> That's very bad indeed. I assumed that the kernel would free
> devm-allocated resources after probe returned -EPROBE_DEFER, so that
> probe could reallocate them the next time it is called. Apparently
> that was wrong. Do you know what would the right approach be in this
> case? Does the kernel preserve the device structure and its
> associated data between the two calls to probe? If so, I could just
> check whether the private data has already been constructed to know
> which state we are in and continue from there.
I'd really expect the devm_ functions to behave as you expect - if they
don't currently we should probably fix them so that they do otherwise
we're going to be in for a lot of surprises and probe deferral becomes a
lot more cumbersome to use.
^ permalink raw reply
* [PATCH 0/2] OMAPDSS: PM runtime fixes for 3.5-rc
From: Archit Taneja @ 2012-07-04 12:55 UTC (permalink / raw)
To: FlorianSchandinat
Cc: tomi.valkeinen, jaswinder.singh, jw, linux-fbdev, linux-omap,
Archit Taneja
Hi Florian,
These are 2 fixes which Tomi had intended to push for the coming rcs. He is
currently on vacation and asked me to post the patches on his behalf.
The first patch ensures that system suspend doesn't break on OMAP if DSS is
enabled. The second patch prevents some warning backtraces when the kernel is
not built with PM runtime.
Could you please queue these for the upcoming rc?
Thanks,
Archit
Tomi Valkeinen (2):
OMAPDSS: Use PM notifiers for system suspend
OMAPDSS: fix warnings if CONFIG_PM_RUNTIME=n
drivers/video/omap2/dss/core.c | 45 +++++++++++++++++++++++++--------------
drivers/video/omap2/dss/dispc.c | 2 +-
drivers/video/omap2/dss/dsi.c | 2 +-
drivers/video/omap2/dss/dss.c | 2 +-
drivers/video/omap2/dss/hdmi.c | 2 +-
drivers/video/omap2/dss/rfbi.c | 2 +-
drivers/video/omap2/dss/venc.c | 2 +-
7 files changed, 35 insertions(+), 22 deletions(-)
--
1.7.9.5
^ permalink raw reply
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