* [PATCH v5] da8xx-fb: allow frame to complete after disabling LCDC
From: Manjunathappa, Prakash @ 2012-08-24 13:25 UTC (permalink / raw)
To: linux-fbdev
Wait for active frame transfer to complete after disabling LCDC.
At the same this wait is not be required when there are sync and
underflow errors.
Patch applies for revision 2 of LCDC present am335x.
More information on disable and reset sequence can be found in
section 13.4.6 of AM335x TRM @www.ti.com/am335x.
Signed-off-by: Manjunathappa, Prakash <prakash.pm@ti.com>
---
Applies on top of fbdev-next of Florian Tobias Schandinat's tree.
Since v4:
Minor nit, removed extra line.
Since v3:
Rely on frame done interrupt instead of polling for it.
Since v2:
Optimized the lcd_disable_raster function.
Since v1:
Changed the commit message, also added link to hardware specification.
drivers/video/da8xx-fb.c | 52 +++++++++++++++++++++++++++++++++++----------
1 files changed, 40 insertions(+), 12 deletions(-)
diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
index 7ae9d53..32f0d06 100644
--- a/drivers/video/da8xx-fb.c
+++ b/drivers/video/da8xx-fb.c
@@ -27,6 +27,7 @@
#include <linux/platform_device.h>
#include <linux/uaccess.h>
#include <linux/interrupt.h>
+#include <linux/wait.h>
#include <linux/clk.h>
#include <linux/cpufreq.h>
#include <linux/console.h>
@@ -48,6 +49,7 @@
#define LCD_PL_LOAD_DONE BIT(6)
#define LCD_FIFO_UNDERFLOW BIT(5)
#define LCD_SYNC_LOST BIT(2)
+#define LCD_FRAME_DONE BIT(0)
/* LCD DMA Control Register */
#define LCD_DMA_BURST_SIZE(x) ((x) << 4)
@@ -135,6 +137,8 @@ static resource_size_t da8xx_fb_reg_base;
static struct resource *lcdc_regs;
static unsigned int lcd_revision;
static irq_handler_t lcdc_irq_handler;
+static wait_queue_head_t frame_done_wq;
+static int frame_done_flag;
static inline unsigned int lcdc_read(unsigned int addr)
{
@@ -288,13 +292,26 @@ static inline void lcd_enable_raster(void)
}
/* Disable the Raster Engine of the LCD Controller */
-static inline void lcd_disable_raster(void)
+static inline void lcd_disable_raster(bool wait_for_frame_done)
{
u32 reg;
+ int ret;
reg = lcdc_read(LCD_RASTER_CTRL_REG);
if (reg & LCD_RASTER_ENABLE)
lcdc_write(reg & ~LCD_RASTER_ENABLE, LCD_RASTER_CTRL_REG);
+ else
+ /* return if already disabled */
+ return;
+
+ if ((wait_for_frame_done = true) && (lcd_revision = LCD_VERSION_2)) {
+ frame_done_flag = 0;
+ ret = wait_event_interruptible_timeout(frame_done_wq,
+ frame_done_flag != 0,
+ msecs_to_jiffies(50));
+ if (ret = 0)
+ pr_err("LCD Controller timed out\n");
+ }
}
static void lcd_blit(int load_mode, struct da8xx_fb_par *par)
@@ -321,7 +338,8 @@ static void lcd_blit(int load_mode, struct da8xx_fb_par *par)
} else {
reg_int = lcdc_read(LCD_INT_ENABLE_SET_REG) |
LCD_V2_END_OF_FRAME0_INT_ENA |
- LCD_V2_END_OF_FRAME1_INT_ENA;
+ LCD_V2_END_OF_FRAME1_INT_ENA |
+ LCD_FRAME_DONE;
lcdc_write(reg_int, LCD_INT_ENABLE_SET_REG);
}
reg_dma |= LCD_DUAL_FRAME_BUFFER_ENABLE;
@@ -638,7 +656,7 @@ static int fb_setcolreg(unsigned regno, unsigned red, unsigned green,
static void lcd_reset(struct da8xx_fb_par *par)
{
/* Disable the Raster if previously Enabled */
- lcd_disable_raster();
+ lcd_disable_raster(false);
/* DMA has to be disabled */
lcdc_write(0, LCD_DMA_CTRL_REG);
@@ -734,7 +752,7 @@ static irqreturn_t lcdc_irq_handler_rev02(int irq, void *arg)
u32 stat = lcdc_read(LCD_MASKED_STAT_REG);
if ((stat & LCD_SYNC_LOST) && (stat & LCD_FIFO_UNDERFLOW)) {
- lcd_disable_raster();
+ lcd_disable_raster(false);
lcdc_write(stat, LCD_MASKED_STAT_REG);
lcd_enable_raster();
} else if (stat & LCD_PL_LOAD_DONE) {
@@ -744,7 +762,7 @@ static irqreturn_t lcdc_irq_handler_rev02(int irq, void *arg)
* interrupt via the following write to the status register. If
* this is done after then one gets multiple PL done interrupts.
*/
- lcd_disable_raster();
+ lcd_disable_raster(false);
lcdc_write(stat, LCD_MASKED_STAT_REG);
@@ -775,6 +793,14 @@ static irqreturn_t lcdc_irq_handler_rev02(int irq, void *arg)
par->vsync_flag = 1;
wake_up_interruptible(&par->vsync_wait);
}
+
+ /* Set only when controller is disabled and at the end of
+ * active frame
+ */
+ if (stat & BIT(0)) {
+ frame_done_flag = 1;
+ wake_up_interruptible(&frame_done_wq);
+ }
}
lcdc_write(0, LCD_END_OF_INT_IND_REG);
@@ -789,7 +815,7 @@ static irqreturn_t lcdc_irq_handler_rev01(int irq, void *arg)
u32 reg_ras;
if ((stat & LCD_SYNC_LOST) && (stat & LCD_FIFO_UNDERFLOW)) {
- lcd_disable_raster();
+ lcd_disable_raster(false);
lcdc_write(stat, LCD_STAT_REG);
lcd_enable_raster();
} else if (stat & LCD_PL_LOAD_DONE) {
@@ -799,7 +825,7 @@ static irqreturn_t lcdc_irq_handler_rev01(int irq, void *arg)
* interrupt via the following write to the status register. If
* this is done after then one gets multiple PL done interrupts.
*/
- lcd_disable_raster();
+ lcd_disable_raster(false);
lcdc_write(stat, LCD_STAT_REG);
@@ -898,7 +924,7 @@ static int lcd_da8xx_cpufreq_transition(struct notifier_block *nb,
if (val = CPUFREQ_POSTCHANGE) {
if (par->lcd_fck_rate != clk_get_rate(par->lcdc_clk)) {
par->lcd_fck_rate = clk_get_rate(par->lcdc_clk);
- lcd_disable_raster();
+ lcd_disable_raster(true);
lcd_calc_clk_divider(par);
lcd_enable_raster();
}
@@ -935,7 +961,7 @@ static int __devexit fb_remove(struct platform_device *dev)
if (par->panel_power_ctrl)
par->panel_power_ctrl(0);
- lcd_disable_raster();
+ lcd_disable_raster(true);
lcdc_write(0, LCD_RASTER_CTRL_REG);
/* disable DMA */
@@ -1051,7 +1077,7 @@ static int cfb_blank(int blank, struct fb_info *info)
if (par->panel_power_ctrl)
par->panel_power_ctrl(0);
- lcd_disable_raster();
+ lcd_disable_raster(true);
break;
default:
ret = -EINVAL;
@@ -1356,8 +1382,10 @@ static int __devinit fb_probe(struct platform_device *device)
if (lcd_revision = LCD_VERSION_1)
lcdc_irq_handler = lcdc_irq_handler_rev01;
- else
+ else {
+ init_waitqueue_head(&frame_done_wq);
lcdc_irq_handler = lcdc_irq_handler_rev02;
+ }
ret = request_irq(par->irq, lcdc_irq_handler, 0,
DRIVER_NAME, par);
@@ -1411,7 +1439,7 @@ static int fb_suspend(struct platform_device *dev, pm_message_t state)
par->panel_power_ctrl(0);
fb_set_suspend(info, 1);
- lcd_disable_raster();
+ lcd_disable_raster(true);
clk_disable(par->lcdc_clk);
console_unlock();
--
1.7.1
^ permalink raw reply related
* Re: [PATCH 02/23] OMAPDSS: outputs: Create and initialize output instances
From: Tomi Valkeinen @ 2012-08-24 13:14 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-omap, linux-fbdev, rob, sumit.semwal
In-Reply-To: <1345528711-27801-3-git-send-email-archit@ti.com>
[-- Attachment #1: Type: text/plain, Size: 3915 bytes --]
On Tue, 2012-08-21 at 11:28 +0530, Archit Taneja wrote:
> Create output instances by having an init function in the probes of the platform
> device drivers for different interfaces. Create a small function for each
> interface to initialize the output entity's fields type and id.
>
> In the probe of each interface driver, the output entities are created before
> the *_probe_pdata() functions intentionally. This is done to ensure that the
> output entity is prepared before the panels connected to the output are
> registered. We need the output entities to be ready because OMAPDSS will try
> to make connections between overlays, managers, outputs and devices during the
> panel's probe.
You're referring to the recheck_connections stuff? I have a patch that
moves that to omapfb side. But of course it doesn't hurt to initialize
the output early.
We should generally do the initialization in output driver's probe more
or less so that we first setup everything related to the output driver,
and after that we register the dssdevs. But I think that's what is
already done.
So, no complaints =).
> Signed-off-by: Archit Taneja <archit@ti.com>
> ---
> drivers/video/omap2/dss/dpi.c | 20 ++++++++++++++++++++
> drivers/video/omap2/dss/dsi.c | 26 ++++++++++++++++++++++++--
> drivers/video/omap2/dss/hdmi.c | 18 ++++++++++++++++++
> drivers/video/omap2/dss/rfbi.c | 19 +++++++++++++++++++
> drivers/video/omap2/dss/sdi.c | 20 ++++++++++++++++++++
> drivers/video/omap2/dss/venc.c | 20 ++++++++++++++++++++
> 6 files changed, 121 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/omap2/dss/dpi.c b/drivers/video/omap2/dss/dpi.c
> index f260343..4eca2e7 100644
> --- a/drivers/video/omap2/dss/dpi.c
> +++ b/drivers/video/omap2/dss/dpi.c
> @@ -408,10 +408,30 @@ static void __init dpi_probe_pdata(struct platform_device *pdev)
> }
> }
>
> +static int __init dpi_init_output(struct platform_device *pdev)
> +{
> + struct omap_dss_output *out;
> +
> + out = dss_create_output(pdev);
> + if (!out)
> + return -ENOMEM;
> +
> + out->id = OMAP_DSS_OUTPUT_DPI;
> + out->type = OMAP_DISPLAY_TYPE_DPI;
> +
> + return 0;
> +}
> +
> static int __init omap_dpi_probe(struct platform_device *pdev)
> {
> + int r;
> +
> mutex_init(&dpi.lock);
>
> + r = dpi_init_output(pdev);
> + if (r)
> + return r;
> +
> dpi_probe_pdata(pdev);
>
> return 0;
> diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
> index 659b6cd..22e0873 100644
> --- a/drivers/video/omap2/dss/dsi.c
> +++ b/drivers/video/omap2/dss/dsi.c
> @@ -4903,6 +4903,23 @@ static void __init dsi_probe_pdata(struct platform_device *dsidev)
> }
> }
>
> +static int __init dsi_init_output(struct platform_device *dsidev,
> + struct dsi_data *dsi)
> +{
> + struct omap_dss_output *out;
> +
> + out = dss_create_output(dsidev);
> + if (!out)
> + return -ENOMEM;
> +
> + out->id = dsi->module_id == 0 ?
> + OMAP_DSS_OUTPUT_DSI1 : OMAP_DSS_OUTPUT_DSI2;
> +
> + out->type = OMAP_DISPLAY_TYPE_DSI;
> +
> + return 0;
As I mentioned in the last email, I think this could be something like:
struct omap_dss_output *out = &dsi->output;
out->pdev = dsidev;
out->id = xxx;
out->type = yyy;
dss_register_output(out);
> +}
> +
> /* DSI1 HW IP initialisation */
> static int __init omap_dsihw_probe(struct platform_device *dsidev)
> {
> @@ -4997,10 +5014,14 @@ static int __init omap_dsihw_probe(struct platform_device *dsidev)
> else
> dsi->num_lanes_supported = 3;
>
> - dsi_probe_pdata(dsidev);
> -
> dsi_runtime_put(dsidev);
>
> + r = dsi_init_output(dsidev, dsi);
> + if (r)
> + goto err_init_output;
> +
> + dsi_probe_pdata(dsidev);
> +
Why do you change the sequence here? Isn't it enough to just add the
init_output before probe_pdata?
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 01/23] OMAPDSS: outputs: Create a new entity called outputs
From: Archit Taneja @ 2012-08-24 12:53 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: Archit Taneja, linux-omap, linux-fbdev, rob, sumit.semwal
In-Reply-To: <1345812069.9287.65.camel@lappyti>
On Friday 24 August 2012 06:11 PM, Tomi Valkeinen wrote:
> On Tue, 2012-08-21 at 11:28 +0530, Archit Taneja wrote:
>
>> diff --git a/drivers/video/omap2/dss/output.c b/drivers/video/omap2/dss/output.c
>> new file mode 100644
>> index 0000000..034ebbe
>> --- /dev/null
>> +++ b/drivers/video/omap2/dss/output.c
>> @@ -0,0 +1,58 @@
>> +/*
>> + * Copyright (C) 2012 Texas Instruments Ltd
>> + * Author: Archit Taneja <archit@ti.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms of the GNU General Public License version 2 as published by
>> + * the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
>> + * more details.
>> + *
>> + * You should have received a copy of the GNU General Public License along with
>> + * this program. If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/slab.h>
>> +
>> +#include <video/omapdss.h>
>> +
>> +#include "dss.h"
>> +
>> +static struct list_head output_list;
>
> You can do:
>
> static LIST_HEAD(output_list);
>
> Then you don't need to initialize it separately.
Oh ok. I'll fix this.
>
>> +
>> +struct omap_dss_output *dss_create_output(struct platform_device *pdev)
>> +{
>> + struct omap_dss_output *out;
>> +
>> + out = kzalloc(sizeof(struct omap_dss_output *), GFP_KERNEL);
>> + if (!out)
>> + return NULL;
>
> A patch that adds kzalloc but no free is always a bit suspicious =).
>
>> +
>> + out->pdev = pdev;
>> +
>> + list_add_tail(&out->list, &output_list);
>> +
>> + return out;
>> +}
>
> Instead of allocating omap_dss_output here, you could let the caller do
> it, and only initialize it here with default values (if that's even
> needed). Then the caller can use kzalloc, or can just embed the stuct
> into its own data-struct, which may be often a better choice.
So output can be in each interface driver's private data, and we just
add that to our list of outputs?
>
>> +struct omap_dss_output *omap_dss_get_output(enum omap_dss_output_id id)
>> +{
>> + struct omap_dss_output *out;
>> +
>> + list_for_each_entry(out, &output_list, list) {
>> + if (out->id = id)
>> + return out;
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +void dss_init_outputs(void)
>> +{
>> + INIT_LIST_HEAD(&output_list);
>> +}
>> diff --git a/include/video/omapdss.h b/include/video/omapdss.h
>> index b868123..0ba613f 100644
>> --- a/include/video/omapdss.h
>> +++ b/include/video/omapdss.h
>> @@ -207,6 +207,16 @@ enum omap_hdmi_flags {
>> OMAP_HDMI_SDA_SCL_EXTERNAL_PULLUP = 1 << 0,
>> };
>>
>> +enum omap_dss_output_id {
>> + OMAP_DSS_OUTPUT_DPI = 1 << 0,
>> + OMAP_DSS_OUTPUT_DBI = 1 << 1,
>> + OMAP_DSS_OUTPUT_SDI = 1 << 2,
>> + OMAP_DSS_OUTPUT_DSI1 = 1 << 3,
>> + OMAP_DSS_OUTPUT_VENC = 1 << 4,
>> + OMAP_DSS_OUTPUT_DSI2 = 1 << 5,
>> + OMAP_DSS_OUTPUT_HDMI = 1 << 6,
>> +};
>
> I'm not sure about this. We already have enum omap_display_type. If you
> need the instance number, you could have that as a separate int field.
>
> Where do you need the output_id?
output_id is used to take care of situations where there our multiple
outputs of the same type, like DSI1 and DSI2. An enum helps when we
check if an overlay manager supports that output instance or not. For
ex, on OMAP4, LCD1 connects to DSI1 and not DSI2.
I add a func called dss_feat_get_supported_outputs(channel) later to
check for this. When setting a new output for a manager, we just do an
'&' to see if the output in question is in the mask of the manager's set
of supported outputs.
>
>> +
>> /* RFBI */
>>
>> struct rfbi_timings {
>> @@ -492,6 +502,24 @@ struct omap_dsi_pin_config {
>> int pins[OMAP_DSS_MAX_DSI_PINS];
>> };
>>
>> +struct omap_dss_output {
>> + struct list_head list;
>> +
>> + /* display type supported by the output */
>> + enum omap_display_type type;
>> +
>> + /* output instance */
>> + enum omap_dss_output_id id;
>
> So instead of omap_dss_output_id, you'd have omap_display_type type and
> int id, which together tell the supported output and also the output
> driver instance.
>
> Tomi
>
^ permalink raw reply
* Re: [PATCH 01/23] OMAPDSS: outputs: Create a new entity called outputs
From: Tomi Valkeinen @ 2012-08-24 12:41 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-omap, linux-fbdev, rob, sumit.semwal
In-Reply-To: <1345528711-27801-2-git-send-email-archit@ti.com>
[-- Attachment #1: Type: text/plain, Size: 3630 bytes --]
On Tue, 2012-08-21 at 11:28 +0530, Archit Taneja wrote:
> diff --git a/drivers/video/omap2/dss/output.c b/drivers/video/omap2/dss/output.c
> new file mode 100644
> index 0000000..034ebbe
> --- /dev/null
> +++ b/drivers/video/omap2/dss/output.c
> @@ -0,0 +1,58 @@
> +/*
> + * Copyright (C) 2012 Texas Instruments Ltd
> + * Author: Archit Taneja <archit@ti.com>
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License version 2 as published by
> + * the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#include <video/omapdss.h>
> +
> +#include "dss.h"
> +
> +static struct list_head output_list;
You can do:
static LIST_HEAD(output_list);
Then you don't need to initialize it separately.
> +
> +struct omap_dss_output *dss_create_output(struct platform_device *pdev)
> +{
> + struct omap_dss_output *out;
> +
> + out = kzalloc(sizeof(struct omap_dss_output *), GFP_KERNEL);
> + if (!out)
> + return NULL;
A patch that adds kzalloc but no free is always a bit suspicious =).
> +
> + out->pdev = pdev;
> +
> + list_add_tail(&out->list, &output_list);
> +
> + return out;
> +}
Instead of allocating omap_dss_output here, you could let the caller do
it, and only initialize it here with default values (if that's even
needed). Then the caller can use kzalloc, or can just embed the stuct
into its own data-struct, which may be often a better choice.
> +struct omap_dss_output *omap_dss_get_output(enum omap_dss_output_id id)
> +{
> + struct omap_dss_output *out;
> +
> + list_for_each_entry(out, &output_list, list) {
> + if (out->id == id)
> + return out;
> + }
> +
> + return NULL;
> +}
> +
> +void dss_init_outputs(void)
> +{
> + INIT_LIST_HEAD(&output_list);
> +}
> diff --git a/include/video/omapdss.h b/include/video/omapdss.h
> index b868123..0ba613f 100644
> --- a/include/video/omapdss.h
> +++ b/include/video/omapdss.h
> @@ -207,6 +207,16 @@ enum omap_hdmi_flags {
> OMAP_HDMI_SDA_SCL_EXTERNAL_PULLUP = 1 << 0,
> };
>
> +enum omap_dss_output_id {
> + OMAP_DSS_OUTPUT_DPI = 1 << 0,
> + OMAP_DSS_OUTPUT_DBI = 1 << 1,
> + OMAP_DSS_OUTPUT_SDI = 1 << 2,
> + OMAP_DSS_OUTPUT_DSI1 = 1 << 3,
> + OMAP_DSS_OUTPUT_VENC = 1 << 4,
> + OMAP_DSS_OUTPUT_DSI2 = 1 << 5,
> + OMAP_DSS_OUTPUT_HDMI = 1 << 6,
> +};
I'm not sure about this. We already have enum omap_display_type. If you
need the instance number, you could have that as a separate int field.
Where do you need the output_id?
> +
> /* RFBI */
>
> struct rfbi_timings {
> @@ -492,6 +502,24 @@ struct omap_dsi_pin_config {
> int pins[OMAP_DSS_MAX_DSI_PINS];
> };
>
> +struct omap_dss_output {
> + struct list_head list;
> +
> + /* display type supported by the output */
> + enum omap_display_type type;
> +
> + /* output instance */
> + enum omap_dss_output_id id;
So instead of omap_dss_output_id, you'd have omap_display_type type and
int id, which together tell the supported output and also the output
driver instance.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
From: Tomi Valkeinen @ 2012-08-24 10:34 UTC (permalink / raw)
To: Alex Courbot
Cc: Thierry Reding, Stephen Warren, Simon Glass, Grant Likely,
Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse,
Arnd Bergmann, Leela Krishna Amudala, linux-tegra@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-fbdev@vger.kernel.org,
devicetree-discuss@lists.ozlabs.org, linux-doc@vger.kernel.org
In-Reply-To: <6044581.2jEzZBWCu1@percival>
[-- Attachment #1: Type: text/plain, Size: 3518 bytes --]
On Fri, 2012-08-24 at 18:24 +0900, Alex Courbot wrote:
> On Tuesday 21 August 2012 17:54:20 Tomi Valkeinen wrote:
> > And as I said, I don't have any problems with some kind of generic power
> > sequences. So the code in the board file could be moved and converted to
> > use the power sequences, if that is better than just plain c code.
>
> My concern now is, provided that all drivers to their job and handle how their
> devices are switched on and off, when (if at all) are encoded power sequences
> better than their equivalent C code? There is the matching database size issue
> that you mentionned, is it a sufficient concern to justify a new kernel feature?
Good question.
I think obviously the worst solution is to have separate .c driver files
for each panel, where the drivers do 99% the same thing.
So the question is how to represent the 1% difference the panels have.
I think it depends on the panels. If it looks like all the panel have,
say, max 2 regulators and one reset/enable gpio, and they are always
enabled in the same order (regulators first, then the gpio), it should
be easy to handle it in the driver without any power sequence framework.
If the panels require more complex setups, then the code in the panel
driver would probably start to form into a power sequence framework, and
it would make sense to have it as a separate framework.
Then again, if the panel setup is complex, it makes me wonder if it'd be
just easier to handle it with c code in a separate driver.
Also, the database size issue is a bit separate issue. There's the db
size problem with or without the framework, if we do not pass the data
from DT.
So as clarification, I see 4 different options:
- Power sequences in DT (as proposed in this series)
- Custom panel data in DT, that the driver uses to power up properly
- Power sequences in a panel database in kernel
- Custom panel data in a panel db in kernel
> On the other hand some devices like panels are typically not used in many
> different appliances, so maybe it is not worth to separate them from their
Yep, this is the reason for my concern with the database size. The DB
could contain 10k panels, of which a board uses one. The rest just waste
memory. But then again, 10k panels is probably not a realistic amount.
It's difficult to guess the amount of memory used by such a database,
though. If it uses, say, 8kB, I'm not sure if it's a reason to panic.
And, as I mentioned, it could be optimized so that the driver throws
away the unneeded panels at __init. Of course here the problem is that
the panel needs to know what panels are needed.
> board definition. As Mark mentionned, having .dtsi files for the DT (and their
> equivalent .h for kernels that use platform data) might be a good middle
> ground.
I guess it's the perfectionist in me that leans toward handling it fully
in the kernel, as I think that's architecturally correct thing to do.
The DT data should contain parameters that can be configured per board,
or nodes (like regulators) that are needed by other parts of the DT
data.
If the only reason why to do it via DT is to avoid the possible size
problem with the panel database, perhaps what we should solve is the
optimization of the database. If that seems unsolvable, then DT approach
could be used as a workaround.
And I have to say I'm not too familiar with DT, so if I'm wrong about
what DT should contain, I'm all ears =).
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 6/8] OMAPDSS: DSI: calculate dsi clock
From: Archit Taneja @ 2012-08-24 9:29 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-omap, linux-fbdev
In-Reply-To: <1345798540.2614.17.camel@deskari>
On Friday 24 August 2012 02:25 PM, Tomi Valkeinen wrote:
> On Fri, 2012-08-24 at 11:46 +0530, Archit Taneja wrote:
>> On Thursday 23 August 2012 07:15 PM, Tomi Valkeinen wrote:
>
>>> + /* pck = TxByteClkHS * datalanes * 8 / bitsperpixel */
>>
>> This formula looks a bit simplified, we aren't considering the header
>> and footers of long packets that will add to the DDR clock. But I guess
>> not considering these would only give a higher pixel clock than needed,
>> which isn't that bad.
>
> Hmm. The TRM (omap4460) gives this formula in "10.3.4.5.12 How to
> Configure the DSI PLL in Video Mode". The headers/footers etc. are
> handled with adjusting the blanking periods so that DISPC and DSI Tline
> times match.
>
> But obviously they are not used for command mode transfers, so perhaps
> you have a point there. Then again, at least in theory, in command mode
> the DISPC pck should be configurable as high as possible because the
> stall mechanism should stop DISPC when DSI has had enough. And so the
> pck calculation is a bit unneeded for cmd mode, we could just configure
> pck to max.
>
> But if it's correct for video mode, and very close for cmd mode, I guess
> it should be fine?
Yes, it's fine, and we shouldn't try to have an unnecessarily high pixel
clock in command mode anyway, that would reduce the amount of
downscaling we could do.
Archit
^ permalink raw reply
* Re: [PATCH v4 1/3] Runtime Interpreted Power Sequences
From: Alex Courbot @ 2012-08-24 9:24 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Thierry Reding, Stephen Warren, Simon Glass, Grant Likely,
Rob Herring, Mark Brown, Anton Vorontsov, David Woodhouse,
Arnd Bergmann, Leela Krishna Amudala, linux-tegra@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-fbdev@vger.kernel.org,
devicetree-discuss@lists.ozlabs.org, linux-doc@vger.kernel.org
In-Reply-To: <1345542860.4085.40.camel@deskari>
On Tuesday 21 August 2012 17:54:20 Tomi Valkeinen wrote:
> > However this also means we'll essentially just be moving the board code.
>
>
> What do you mean "just"? Wasn't the point of the whole "arm board file
> mess" to get rid of the code from the board files? If the code in the
> board file is device specific code, not board specific, then the driver
> of the device is a logical place to place it.
I think Tomi has a point here - these sequences were not belonging to the
board code in the first place. They are definitely tied to the device, hence
should have been handled by the driver all along, with the board code
assigning the correct resources to the device (like the vast majority of
device drivers do).
> And as I said, I don't have any problems with some kind of generic power
> sequences. So the code in the board file could be moved and converted to
> use the power sequences, if that is better than just plain c code.
My concern now is, provided that all drivers to their job and handle how their
devices are switched on and off, when (if at all) are encoded power sequences
better than their equivalent C code? There is the matching database size issue
that you mentionned, is it a sufficient concern to justify a new kernel feature?
On the other hand some devices like panels are typically not used in many
different appliances, so maybe it is not worth to separate them from their
board definition. As Mark mentionned, having .dtsi files for the DT (and their
equivalent .h for kernels that use platform data) might be a good middle
ground.
But the line is really tight between what is code and what is data here.
Alex.
^ permalink raw reply
* Re: [PATCH 6/8] OMAPDSS: DSI: calculate dsi clock
From: Tomi Valkeinen @ 2012-08-24 8:55 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-omap, linux-fbdev
In-Reply-To: <50371C47.30708@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1143 bytes --]
On Fri, 2012-08-24 at 11:46 +0530, Archit Taneja wrote:
> On Thursday 23 August 2012 07:15 PM, Tomi Valkeinen wrote:
> > + /* pck = TxByteClkHS * datalanes * 8 / bitsperpixel */
>
> This formula looks a bit simplified, we aren't considering the header
> and footers of long packets that will add to the DDR clock. But I guess
> not considering these would only give a higher pixel clock than needed,
> which isn't that bad.
Hmm. The TRM (omap4460) gives this formula in "10.3.4.5.12 How to
Configure the DSI PLL in Video Mode". The headers/footers etc. are
handled with adjusting the blanking periods so that DISPC and DSI Tline
times match.
But obviously they are not used for command mode transfers, so perhaps
you have a point there. Then again, at least in theory, in command mode
the DISPC pck should be configurable as high as possible because the
stall mechanism should stop DISPC when DSI has had enough. And so the
pck calculation is a bit unneeded for cmd mode, we could just configure
pck to max.
But if it's correct for video mode, and very close for cmd mode, I guess
it should be fine?
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 4/8] OMAPDSS: HDMI: use vdda_hdmi_dac
From: Tomi Valkeinen @ 2012-08-24 6:41 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-omap, linux-fbdev
In-Reply-To: <50371D27.1010707@ti.com>
[-- Attachment #1: Type: text/plain, Size: 2703 bytes --]
On Fri, 2012-08-24 at 11:50 +0530, Archit Taneja wrote:
> On Thursday 23 August 2012 07:15 PM, Tomi Valkeinen wrote:
> > The HDMI driver requires vdda_hdmi_dac power for operation, but does not
> > enable it. This has worked because the regulator has been always
> > enabled.
> >
> > But this may not always be the case, as I encountered when implementing
> > HDMI device tree support.
> >
> > This patch changes the HDMI driver to use the vdda_hdmi_dac.
> >
> > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > ---
> > drivers/video/omap2/dss/hdmi.c | 23 +++++++++++++++++++++++
> > 1 file changed, 23 insertions(+)
> >
> > diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
> > index 96a6e29..ccfc677 100644
> > --- a/drivers/video/omap2/dss/hdmi.c
> > +++ b/drivers/video/omap2/dss/hdmi.c
> > @@ -33,6 +33,7 @@
> > #include <linux/pm_runtime.h>
> > #include <linux/clk.h>
> > #include <linux/gpio.h>
> > +#include <linux/regulator/consumer.h>
> > #include <video/omapdss.h>
> >
> > #include "ti_hdmi.h"
> > @@ -62,6 +63,7 @@ static struct {
> > struct hdmi_ip_data ip_data;
> >
> > struct clk *sys_clk;
> > + struct regulator *vdda_hdmi_dac_reg;
> >
> > int ct_cp_hpd_gpio;
> > int ls_oe_gpio;
> > @@ -331,6 +333,19 @@ static int __init hdmi_init_display(struct omap_dss_device *dssdev)
> >
> > dss_init_hdmi_ip_ops(&hdmi.ip_data);
> >
> > + if (hdmi.vdda_hdmi_dac_reg == NULL) {
> > + struct regulator *reg;
> > +
> > + reg = devm_regulator_get(&hdmi.pdev->dev, "vdda_hdmi_dac");
>
> There is no corresponding devm_regulator_put() call here, I guess that's
> what devm_* calls are supposed to help us with. But the only place I saw
> the usage of dev_regulator_get() is here:
>
> sound/soc/ux500/ux500_msp_dai.c
>
> And here, they are doing devm_regulator_put() calls to, so I was
> wondering what the deal is.
No idea. But there are other places also: sound/soc/codecs/wm5100.c,
sound/soc/codecs/wm8996.c, sound/soc/soc-dapm.c, and those don't use
put().
Anyway, I think it's quite clear how devm_* functions are used, so the
put call in ux500_msp_dai.c is probably just a mistake.
I also want to mention that doing the regulator_get in
hdmi_init_display() is somewhat strange, but the point is to do the
regulator_get only if a hdmi display has been defined in the board file.
If we did it unconditionally in hdmi's probe, we'd try to get the
regulator always, and it could be that there's no regulator if the hdmi
is not used on a particular board. This is again something that feels
hackish, and I'd like to find a cleaner solution to it.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 4/8] OMAPDSS: HDMI: use vdda_hdmi_dac
From: Archit Taneja @ 2012-08-24 6:32 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-omap, linux-fbdev
In-Reply-To: <1345729514-2441-5-git-send-email-tomi.valkeinen@ti.com>
On Thursday 23 August 2012 07:15 PM, Tomi Valkeinen wrote:
> The HDMI driver requires vdda_hdmi_dac power for operation, but does not
> enable it. This has worked because the regulator has been always
> enabled.
>
> But this may not always be the case, as I encountered when implementing
> HDMI device tree support.
>
> This patch changes the HDMI driver to use the vdda_hdmi_dac.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
> drivers/video/omap2/dss/hdmi.c | 23 +++++++++++++++++++++++
> 1 file changed, 23 insertions(+)
>
> diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
> index 96a6e29..ccfc677 100644
> --- a/drivers/video/omap2/dss/hdmi.c
> +++ b/drivers/video/omap2/dss/hdmi.c
> @@ -33,6 +33,7 @@
> #include <linux/pm_runtime.h>
> #include <linux/clk.h>
> #include <linux/gpio.h>
> +#include <linux/regulator/consumer.h>
> #include <video/omapdss.h>
>
> #include "ti_hdmi.h"
> @@ -62,6 +63,7 @@ static struct {
> struct hdmi_ip_data ip_data;
>
> struct clk *sys_clk;
> + struct regulator *vdda_hdmi_dac_reg;
>
> int ct_cp_hpd_gpio;
> int ls_oe_gpio;
> @@ -331,6 +333,19 @@ static int __init hdmi_init_display(struct omap_dss_device *dssdev)
>
> dss_init_hdmi_ip_ops(&hdmi.ip_data);
>
> + if (hdmi.vdda_hdmi_dac_reg = NULL) {
> + struct regulator *reg;
> +
> + reg = devm_regulator_get(&hdmi.pdev->dev, "vdda_hdmi_dac");
There is no corresponding devm_regulator_put() call here, I guess that's
what devm_* calls are supposed to help us with. But the only place I saw
the usage of dev_regulator_get() is here:
sound/soc/ux500/ux500_msp_dai.c
And here, they are doing devm_regulator_put() calls to, so I was
wondering what the deal is.
Archit
^ permalink raw reply
* Re: [PATCH 6/8] OMAPDSS: DSI: calculate dsi clock
From: Archit Taneja @ 2012-08-24 6:28 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-omap, linux-fbdev
In-Reply-To: <1345729514-2441-7-git-send-email-tomi.valkeinen@ti.com>
On Thursday 23 August 2012 07:15 PM, Tomi Valkeinen wrote:
> Currently the way to configure clocks related to DSI (both DSI and DISPC
> clocks) happens via omapdss platform data. The reason for this is that
> configuring the DSS clocks is a very complex problem, and it's
> impossible for the SW to know requirements about things like
> interference.
>
> However, for general cases it should be fine to calculate the dividers
> for clocks in the SW. The calculated clocks are probably not perfect,
> but should work.
>
> This patch adds support to calculate the dividers when using DSI command
> mode panels. The panel gives the required DDR clock rate and LP clock
> rate, and the DSI driver configures itself and DISPC accordingly.
>
> This patch is somewhat ugly, though. The code does its job by modifying
> the platform data where the clock dividers would be if the board file
> gave them. This is not how it's going to be in the future, but allows us
> to have quite simple patch and keep the backward compatibility.
>
> It also allows the developer to still give the exact dividers from the
> board file when there's need for that, as long as the panel driver does
> not override them.
>
> There are also other areas for improvement. For example, it would be
> better if the panel driver could ask for a DSI clock in a certain range,
> as, at least command mode panels, the panel can work fine with many
> different clock speeds.
>
> While the patch is not perfect, it allows us to remove the hardcoded
> clock dividers from the board file, making it easier to bring up a new
> panel and to use device tree from omapdss.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
> drivers/video/omap2/displays/panel-taal.c | 6 ++
> drivers/video/omap2/dss/dsi.c | 126 +++++++++++++++++++++++++++++
> include/video/omapdss.h | 2 +
> 3 files changed, 134 insertions(+)
>
> diff --git a/drivers/video/omap2/displays/panel-taal.c b/drivers/video/omap2/displays/panel-taal.c
> index 77aed0e..ddda96a 100644
> --- a/drivers/video/omap2/displays/panel-taal.c
> +++ b/drivers/video/omap2/displays/panel-taal.c
> @@ -1065,6 +1065,12 @@ static int taal_power_on(struct omap_dss_device *dssdev)
> omapdss_dsi_set_pixel_format(dssdev, OMAP_DSS_DSI_FMT_RGB888);
> omapdss_dsi_set_operation_mode(dssdev, OMAP_DSS_DSI_CMD_MODE);
>
> + r = omapdss_dsi_set_clocks(dssdev, 216000000, 10000000);
> + if (r) {
> + dev_err(&dssdev->dev, "failed to set HS and LP clocks\n");
> + goto err0;
> + }
> +
> r = omapdss_dsi_display_enable(dssdev);
> if (r) {
> dev_err(&dssdev->dev, "failed to enable DSI\n");
> diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
> index 96d0024..340c832 100644
> --- a/drivers/video/omap2/dss/dsi.c
> +++ b/drivers/video/omap2/dss/dsi.c
> @@ -1454,6 +1454,68 @@ found:
> return 0;
> }
>
> +static int dsi_pll_calc_ddrfreq(struct platform_device *dsidev,
> + unsigned long req_clk, struct dsi_clock_info *cinfo)
> +{
> + struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev);
> + struct dsi_clock_info cur, best;
> + unsigned long dss_sys_clk, max_dss_fck, max_dsi_fck;
> + unsigned long req_clkin4ddr;
> +
> + DSSDBG("dsi_pll_calc_ddrfreq\n");
> +
> + dss_sys_clk = clk_get_rate(dsi->sys_clk);
> +
> + max_dss_fck = dss_feat_get_param_max(FEAT_PARAM_DSS_FCK);
> + max_dsi_fck = dss_feat_get_param_max(FEAT_PARAM_DSI_FCK);
> +
> + memset(&best, 0, sizeof(best));
> + memset(&cur, 0, sizeof(cur));
> +
> + cur.clkin = dss_sys_clk;
> +
> + req_clkin4ddr = req_clk * 4;
> +
> + for (cur.regn = 1; cur.regn < dsi->regn_max; ++cur.regn) {
> + cur.fint = cur.clkin / cur.regn;
> +
> + if (cur.fint > dsi->fint_max || cur.fint < dsi->fint_min)
> + continue;
> +
> + /* DSIPHY(MHz) = (2 * regm / regn) * clkin */
> + for (cur.regm = 1; cur.regm < dsi->regm_max; ++cur.regm) {
> + unsigned long a, b;
> +
> + a = 2 * cur.regm * (cur.clkin/1000);
> + b = cur.regn;
> + cur.clkin4ddr = a / b * 1000;
> +
> + if (cur.clkin4ddr > 1800 * 1000 * 1000)
> + break;
> +
> + if (abs(cur.clkin4ddr - req_clkin4ddr) <
> + abs(best.clkin4ddr - req_clkin4ddr)) {
> + best = cur;
> + DSSDBG("best %ld\n", best.clkin4ddr);
> + }
> +
> + if (cur.clkin4ddr = req_clkin4ddr)
> + goto found;
> + }
> + }
> +found:
> + best.regm_dispc = DIV_ROUND_UP(best.clkin4ddr, max_dss_fck);
> + best.dsi_pll_hsdiv_dispc_clk = best.clkin4ddr / best.regm_dispc;
> +
> + best.regm_dsi = DIV_ROUND_UP(best.clkin4ddr, max_dsi_fck);
> + best.dsi_pll_hsdiv_dsi_clk = best.clkin4ddr / best.regm_dsi;
> +
> + if (cinfo)
> + *cinfo = best;
> +
> + return 0;
> +}
> +
> int dsi_pll_set_clock_div(struct platform_device *dsidev,
> struct dsi_clock_info *cinfo)
> {
> @@ -4110,6 +4172,70 @@ int omapdss_dsi_configure_pins(struct omap_dss_device *dssdev,
> }
> EXPORT_SYMBOL(omapdss_dsi_configure_pins);
>
> +int omapdss_dsi_set_clocks(struct omap_dss_device *dssdev,
> + unsigned long ddr_clk, unsigned long lp_clk)
> +{
> + struct platform_device *dsidev = dsi_get_dsidev_from_dssdev(dssdev);
> + struct dsi_data *dsi = dsi_get_dsidrv_data(dsidev);
> + struct dsi_clock_info cinfo;
> + struct dispc_clock_info dispc_cinfo;
> + unsigned lp_clk_div;
> + unsigned long dsi_fclk;
> + int bpp = dsi_get_pixel_size(dssdev->panel.dsi_pix_fmt);
> + unsigned long pck;
> + int r;
> +
> + DSSDBGF("ddr_clk %lu, lp_clk %lu", ddr_clk, lp_clk);
> +
> + mutex_lock(&dsi->lock);
> +
> + r = dsi_pll_calc_ddrfreq(dsidev, ddr_clk, &cinfo);
> + if (r)
> + goto err;
> +
> + dssdev->clocks.dsi.regn = cinfo.regn;
> + dssdev->clocks.dsi.regm = cinfo.regm;
> + dssdev->clocks.dsi.regm_dispc = cinfo.regm_dispc;
> + dssdev->clocks.dsi.regm_dsi = cinfo.regm_dsi;
> +
> +
> + dsi_fclk = cinfo.dsi_pll_hsdiv_dsi_clk;
> + lp_clk_div = DIV_ROUND_UP(dsi_fclk, lp_clk * 2);
> +
> + dssdev->clocks.dsi.lp_clk_div = lp_clk_div;
> +
> + /* pck = TxByteClkHS * datalanes * 8 / bitsperpixel */
This formula looks a bit simplified, we aren't considering the header
and footers of long packets that will add to the DDR clock. But I guess
not considering these would only give a higher pixel clock than needed,
which isn't that bad.
> +
> + pck = cinfo.clkin4ddr / 16 * (dsi->num_lanes_used - 1) * 8 / bpp;
> +
> + DSSDBG("finding dispc dividers for pck %lu\n", pck);
> +
> + dispc_find_clk_divs(pck, cinfo.dsi_pll_hsdiv_dispc_clk, &dispc_cinfo);
> +
> + dssdev->clocks.dispc.channel.lck_div = dispc_cinfo.lck_div;
> + dssdev->clocks.dispc.channel.pck_div = dispc_cinfo.pck_div;
> +
> +
one unnecessary new line above.
<snip>
Archit
^ permalink raw reply
* Re: [PATCH 1/8] OMAPDSS: HDMI: Move GPIO handling to HDMI driver
From: Tomi Valkeinen @ 2012-08-24 6:28 UTC (permalink / raw)
To: Archit Taneja; +Cc: linux-omap, linux-fbdev, Tony Lindgren
In-Reply-To: <50371899.6030706@ti.com>
[-- Attachment #1: Type: text/plain, Size: 2469 bytes --]
On Fri, 2012-08-24 at 11:30 +0530, Archit Taneja wrote:
> On Thursday 23 August 2012 07:15 PM, Tomi Valkeinen wrote:
> > We currently manage HDMI GPIOs in the board files via
> > platform_enable/disable calls. This won't work with device tree, and in
> > any case the correct place to manage the GPIOs is in the HDMI driver.
> >
> > This patch moves the handling of the GPIOs to the HDMI driver. The GPIO
> > handling is moved to the common hdmi.c file, and this probably needs to
> > be revisited when adding OMAP5 HDMI support to see if the GPIO handling
> > needs to be moved to IP specific files.
<snip>
> > static int __init hdmi_init_display(struct omap_dss_device *dssdev)
> > {
> > + int r;
> > +
> > + struct gpio gpios[] = {
> > + { hdmi.ct_cp_hpd_gpio, GPIOF_OUT_INIT_LOW, "hdmi_ct_cp_hpd" },
> > + { hdmi.ls_oe_gpio, GPIOF_OUT_INIT_LOW, "hdmi_ls_oe" },
> > + { hdmi.hpd_gpio, GPIOF_DIR_IN, "hdmi_hpd" },
> > + };
> > +
> > DSSDBG("init_display\n");
> >
> > dss_init_hdmi_ip_ops(&hdmi.ip_data);
> > +
> > + r = gpio_request_array(gpios, ARRAY_SIZE(gpios));
> > + if (r)
> > + return r;
> > +
>
> Is there a reason to request these gpios in hdmi_init_display()? Why
> can't these be requested simply in the probe of the hdmi platform
> device. These gpios are sort of specific to the hdmi output on OMAP,
> aren't they?
Well, it is a bit ugly, agreed. But I'm not sure it can be helped
easily.
First, the struct omap_dss_hdmi_data where the gpios are passed from
board file, is really "panel" platform data. I.e. it's attached to the
hdmi dssdev, and thus we can't handle it in hdmi output driver's probe,
as we don't have dssdevs there.
Second, the gpios are actually not OMAP HDMI stuff. They belong to the
tpd12s015 chip, sitting between OMAP HDMI output and the HDMI connector,
which is the ESD/level shifter/whatnot (I don't remember exactly what it
does =). So making the GPIOs a property of the OMAP HDMI device wouldn't
be right either.
tpd12s015 is rather simple chip, but it still has the gpios and require
special handling. I think the only way to properly handle this would be
to have a chain of external display devices, and the tpd12s015 would be
handled by a separate driver. Until then, the gpio handling is quite
hacky.
We could move the gpios to omapdss's platform data, but it would still
be wrong, and I'm not sure if it'd be any cleaner hack.
Tomi
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH 1/8] OMAPDSS: HDMI: Move GPIO handling to HDMI driver
From: Archit Taneja @ 2012-08-24 6:12 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-omap, linux-fbdev, Tony Lindgren
In-Reply-To: <1345729514-2441-2-git-send-email-tomi.valkeinen@ti.com>
On Thursday 23 August 2012 07:15 PM, Tomi Valkeinen wrote:
> We currently manage HDMI GPIOs in the board files via
> platform_enable/disable calls. This won't work with device tree, and in
> any case the correct place to manage the GPIOs is in the HDMI driver.
>
> This patch moves the handling of the GPIOs to the HDMI driver. The GPIO
> handling is moved to the common hdmi.c file, and this probably needs to
> be revisited when adding OMAP5 HDMI support to see if the GPIO handling
> needs to be moved to IP specific files.
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Cc: Tony Lindgren <tony@atomide.com>
> ---
> arch/arm/mach-omap2/board-4430sdp.c | 27 +-----------
> arch/arm/mach-omap2/board-omap4panda.c | 27 +-----------
> drivers/video/omap2/dss/hdmi.c | 75 +++++++++++++++++++++++---------
> include/video/omapdss.h | 2 +
> 4 files changed, 61 insertions(+), 70 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/board-4430sdp.c b/arch/arm/mach-omap2/board-4430sdp.c
> index 8e17284..852e05c 100644
> --- a/arch/arm/mach-omap2/board-4430sdp.c
> +++ b/arch/arm/mach-omap2/board-4430sdp.c
> @@ -601,29 +601,6 @@ static void __init omap_sfh7741prox_init(void)
> __func__, OMAP4_SFH7741_ENABLE_GPIO, error);
> }
>
> -static struct gpio sdp4430_hdmi_gpios[] = {
> - { HDMI_GPIO_CT_CP_HPD, GPIOF_OUT_INIT_HIGH, "hdmi_gpio_ct_cp_hpd" },
> - { HDMI_GPIO_LS_OE, GPIOF_OUT_INIT_HIGH, "hdmi_gpio_ls_oe" },
> - { HDMI_GPIO_HPD, GPIOF_DIR_IN, "hdmi_gpio_hpd" },
> -};
> -
> -static int sdp4430_panel_enable_hdmi(struct omap_dss_device *dssdev)
> -{
> - int status;
> -
> - status = gpio_request_array(sdp4430_hdmi_gpios,
> - ARRAY_SIZE(sdp4430_hdmi_gpios));
> - if (status)
> - pr_err("%s: Cannot request HDMI GPIOs\n", __func__);
> -
> - return status;
> -}
> -
> -static void sdp4430_panel_disable_hdmi(struct omap_dss_device *dssdev)
> -{
> - gpio_free_array(sdp4430_hdmi_gpios, ARRAY_SIZE(sdp4430_hdmi_gpios));
> -}
> -
> static struct nokia_dsi_panel_data dsi1_panel = {
> .name = "taal",
> .reset_gpio = 102,
> @@ -718,6 +695,8 @@ static struct omap_dss_device sdp4430_lcd2_device = {
> };
>
> static struct omap_dss_hdmi_data sdp4430_hdmi_data = {
> + .ct_cp_hpd_gpio = HDMI_GPIO_CT_CP_HPD,
> + .ls_oe_gpio = HDMI_GPIO_LS_OE,
> .hpd_gpio = HDMI_GPIO_HPD,
> };
>
> @@ -725,8 +704,6 @@ static struct omap_dss_device sdp4430_hdmi_device = {
> .name = "hdmi",
> .driver_name = "hdmi_panel",
> .type = OMAP_DISPLAY_TYPE_HDMI,
> - .platform_enable = sdp4430_panel_enable_hdmi,
> - .platform_disable = sdp4430_panel_disable_hdmi,
> .channel = OMAP_DSS_CHANNEL_DIGIT,
> .data = &sdp4430_hdmi_data,
> };
> diff --git a/arch/arm/mach-omap2/board-omap4panda.c b/arch/arm/mach-omap2/board-omap4panda.c
> index 982fb26..5415faa 100644
> --- a/arch/arm/mach-omap2/board-omap4panda.c
> +++ b/arch/arm/mach-omap2/board-omap4panda.c
> @@ -405,30 +405,9 @@ static struct omap_dss_device omap4_panda_dvi_device = {
> .channel = OMAP_DSS_CHANNEL_LCD2,
> };
>
> -static struct gpio panda_hdmi_gpios[] = {
> - { HDMI_GPIO_CT_CP_HPD, GPIOF_OUT_INIT_HIGH, "hdmi_gpio_ct_cp_hpd" },
> - { HDMI_GPIO_LS_OE, GPIOF_OUT_INIT_HIGH, "hdmi_gpio_ls_oe" },
> - { HDMI_GPIO_HPD, GPIOF_DIR_IN, "hdmi_gpio_hpd" },
> -};
> -
> -static int omap4_panda_panel_enable_hdmi(struct omap_dss_device *dssdev)
> -{
> - int status;
> -
> - status = gpio_request_array(panda_hdmi_gpios,
> - ARRAY_SIZE(panda_hdmi_gpios));
> - if (status)
> - pr_err("Cannot request HDMI GPIOs\n");
> -
> - return status;
> -}
> -
> -static void omap4_panda_panel_disable_hdmi(struct omap_dss_device *dssdev)
> -{
> - gpio_free_array(panda_hdmi_gpios, ARRAY_SIZE(panda_hdmi_gpios));
> -}
> -
> static struct omap_dss_hdmi_data omap4_panda_hdmi_data = {
> + .ct_cp_hpd_gpio = HDMI_GPIO_CT_CP_HPD,
> + .ls_oe_gpio = HDMI_GPIO_LS_OE,
> .hpd_gpio = HDMI_GPIO_HPD,
> };
>
> @@ -436,8 +415,6 @@ static struct omap_dss_device omap4_panda_hdmi_device = {
> .name = "hdmi",
> .driver_name = "hdmi_panel",
> .type = OMAP_DISPLAY_TYPE_HDMI,
> - .platform_enable = omap4_panda_panel_enable_hdmi,
> - .platform_disable = omap4_panda_panel_disable_hdmi,
> .channel = OMAP_DSS_CHANNEL_DIGIT,
> .data = &omap4_panda_hdmi_data,
> };
> diff --git a/drivers/video/omap2/dss/hdmi.c b/drivers/video/omap2/dss/hdmi.c
> index 0cdf246..4fbe271 100644
> --- a/drivers/video/omap2/dss/hdmi.c
> +++ b/drivers/video/omap2/dss/hdmi.c
> @@ -32,6 +32,7 @@
> #include <linux/platform_device.h>
> #include <linux/pm_runtime.h>
> #include <linux/clk.h>
> +#include <linux/gpio.h>
> #include <video/omapdss.h>
>
> #include "ti_hdmi.h"
> @@ -61,6 +62,10 @@ static struct {
> struct hdmi_ip_data ip_data;
>
> struct clk *sys_clk;
> +
> + int ct_cp_hpd_gpio;
> + int ls_oe_gpio;
> + int hpd_gpio;
> } hdmi;
>
> /*
> @@ -314,12 +319,34 @@ static void hdmi_runtime_put(void)
>
> static int __init hdmi_init_display(struct omap_dss_device *dssdev)
> {
> + int r;
> +
> + struct gpio gpios[] = {
> + { hdmi.ct_cp_hpd_gpio, GPIOF_OUT_INIT_LOW, "hdmi_ct_cp_hpd" },
> + { hdmi.ls_oe_gpio, GPIOF_OUT_INIT_LOW, "hdmi_ls_oe" },
> + { hdmi.hpd_gpio, GPIOF_DIR_IN, "hdmi_hpd" },
> + };
> +
> DSSDBG("init_display\n");
>
> dss_init_hdmi_ip_ops(&hdmi.ip_data);
> +
> + r = gpio_request_array(gpios, ARRAY_SIZE(gpios));
> + if (r)
> + return r;
> +
Is there a reason to request these gpios in hdmi_init_display()? Why
can't these be requested simply in the probe of the hdmi platform
device. These gpios are sort of specific to the hdmi output on OMAP,
aren't they?
<snip>
Archit
^ permalink raw reply
* Re: [PATCH 8/8] OMAPDSS: fix use of dssdev->caps
From: Archit Taneja @ 2012-08-24 5:52 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-omap, linux-fbdev
In-Reply-To: <1345729514-2441-9-git-send-email-tomi.valkeinen@ti.com>
On Thursday 23 August 2012 07:15 PM, Tomi Valkeinen wrote:
> Recent commit dca2b1522ccab28d03fb79f6e70e70ea78033d52 (OMAPDSS: DSI:
> Maintain copy of operation mode in driver data) broke DSI for video mode
> displays. The commit changed the way dssdev->caps are initialized, and
> the result was that every DSI display is initialized with manual-update
> and tear-elim caps.
Ah, I didn't realise that. Thanks for catching this.
>
> The code that sets dssdev->caps is not very good, even when fixed.
> omapdss driver shouldn't be writing dssdev->caps at all.
>
> This patch fixes the problem with video mode displays by moving the
> initialization of dssdev->caps to the panel driver. The same change is
> done for RFBI.
Yes, it makes more sense to configure these in the panel driver.
Archit
>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
> drivers/video/omap2/displays/panel-n8x0.c | 1 +
> drivers/video/omap2/displays/panel-taal.c | 2 ++
> drivers/video/omap2/dss/dsi.c | 5 -----
> drivers/video/omap2/dss/rfbi.c | 1 -
> 4 files changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/video/omap2/displays/panel-n8x0.c b/drivers/video/omap2/displays/panel-n8x0.c
> index 17ae85e..3fc5ad0 100644
> --- a/drivers/video/omap2/displays/panel-n8x0.c
> +++ b/drivers/video/omap2/displays/panel-n8x0.c
> @@ -489,6 +489,7 @@ static int n8x0_panel_probe(struct omap_dss_device *dssdev)
> dssdev->panel.timings.y_res = 480;
> dssdev->ctrl.pixel_size = 16;
> dssdev->ctrl.rfbi_timings = n8x0_panel_timings;
> + dssdev->caps = OMAP_DSS_DISPLAY_CAP_MANUAL_UPDATE;
>
> memset(&props, 0, sizeof(props));
> props.max_brightness = 127;
> diff --git a/drivers/video/omap2/displays/panel-taal.c b/drivers/video/omap2/displays/panel-taal.c
> index ddda96a..7b2d7bb 100644
> --- a/drivers/video/omap2/displays/panel-taal.c
> +++ b/drivers/video/omap2/displays/panel-taal.c
> @@ -884,6 +884,8 @@ static int taal_probe(struct omap_dss_device *dssdev)
>
> dssdev->panel.timings = panel_config->timings;
> dssdev->panel.dsi_pix_fmt = OMAP_DSS_DSI_FMT_RGB888;
> + dssdev->caps = OMAP_DSS_DISPLAY_CAP_MANUAL_UPDATE |
> + OMAP_DSS_DISPLAY_CAP_TEAR_ELIM;
>
> td = kzalloc(sizeof(*td), GFP_KERNEL);
> if (!td) {
> diff --git a/drivers/video/omap2/dss/dsi.c b/drivers/video/omap2/dss/dsi.c
> index 340c832..254666f 100644
> --- a/drivers/video/omap2/dss/dsi.c
> +++ b/drivers/video/omap2/dss/dsi.c
> @@ -4866,11 +4866,6 @@ static int __init dsi_init_display(struct omap_dss_device *dssdev)
>
> DSSDBG("DSI init\n");
>
> - if (dsi->mode = OMAP_DSS_DSI_CMD_MODE) {
> - dssdev->caps = OMAP_DSS_DISPLAY_CAP_MANUAL_UPDATE |
> - OMAP_DSS_DISPLAY_CAP_TEAR_ELIM;
> - }
> -
> if (dsi->vdds_dsi_reg = NULL) {
> struct regulator *vdds_dsi;
>
> diff --git a/drivers/video/omap2/dss/rfbi.c b/drivers/video/omap2/dss/rfbi.c
> index 5a9c0e9..2e520d3 100644
> --- a/drivers/video/omap2/dss/rfbi.c
> +++ b/drivers/video/omap2/dss/rfbi.c
> @@ -939,7 +939,6 @@ EXPORT_SYMBOL(omapdss_rfbi_display_disable);
> static int __init rfbi_init_display(struct omap_dss_device *dssdev)
> {
> rfbi.dssdev[dssdev->phy.rfbi.channel] = dssdev;
> - dssdev->caps = OMAP_DSS_DISPLAY_CAP_MANUAL_UPDATE;
> return 0;
> }
>
>
^ permalink raw reply
* [PATCH] video: bfin-lq035q1: use module_platform_driver
From: Devendra Naga @ 2012-08-23 21:55 UTC (permalink / raw)
To: linux-fbdev
the driver's module init and exit functions are calling
platform_driver_register and platform_driver_unregister and doing nothing
else.
This same as that of the module_platform_driver,
remove this init and exit functions and use the module_platform_driver instead
Signed-off-by: Devendra Naga <develkernel412222@gmail.com>
---
drivers/video/bfin-lq035q1-fb.c | 12 +-----------
1 file changed, 1 insertion(+), 11 deletions(-)
diff --git a/drivers/video/bfin-lq035q1-fb.c b/drivers/video/bfin-lq035q1-fb.c
index 353c02f..1b21519 100644
--- a/drivers/video/bfin-lq035q1-fb.c
+++ b/drivers/video/bfin-lq035q1-fb.c
@@ -853,17 +853,7 @@ static struct platform_driver bfin_lq035q1_driver = {
},
};
-static int __init bfin_lq035q1_driver_init(void)
-{
- return platform_driver_register(&bfin_lq035q1_driver);
-}
-module_init(bfin_lq035q1_driver_init);
-
-static void __exit bfin_lq035q1_driver_cleanup(void)
-{
- platform_driver_unregister(&bfin_lq035q1_driver);
-}
-module_exit(bfin_lq035q1_driver_cleanup);
+module_platform_driver(bfin_lq035q1_driver);
MODULE_DESCRIPTION("Blackfin TFT LCD Driver");
MODULE_LICENSE("GPL");
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCHv3 3/9] serial: vt8500: Add devicetree support for
From: Rob Landley @ 2012-08-23 21:29 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <201208220644.18059.arnd@arndb.de>
On 08/22/2012 01:44 AM, Arnd Bergmann wrote:
> On Wednesday 22 August 2012, Tony Prisk wrote:
>> The original patch was very simple, but I revisited it to fix other
>> issues and forgot to add the relevant comments.
>>
>> Port size is changed to fix a problem - WM8505 actually had 6 uart's
>> defined in platform data but the vt8500_ports variable was only 4.
>>
>> I have added devicetree port id support as well.
>
> If you do multiple things in one driver, you should normally send multiple
> patches as well, each with a description why that change is done.
> It may seem silly at first to send out a one-line patch next to a 100-line
> patch for the same file, but those cases are actually the ones where it's
> most important.
Think of us poor git-bisect monkeys who have no idea why something broke
but can (purely mechanically) figure out which commit did it. If it's a
patch that does three unrelated things, we're kinda stuck.
Rob
--
GNU/Linux isn't: Linux=GPLv2, GNU=GPLv3+, they can't share code.
Either it's "mere aggregation", or a license violation. Pick one.
^ permalink raw reply
* [PATCH] pwm-imx: Fix config / enable / disable
From: Benoît Thébaudeau @ 2012-08-23 20:58 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <20120823191108.GB8127@avionic-0098.mockup.avionic-design.de>
imx_pwm_config() did not enable the PWM IP clock while accessing the registers.
Hence, a call to pwm_config() had no effect before pwm_enable() had been called,
which does not comply to the PWM API.
Moreover, calling pwm_disable() then pwm_enable() must be a transparent
operation.
This fixes the first setting of brightness through sysfs that had no effect with
leds-pwm.
Cc: Thierry Reding <thierry.reding@avionic-design.de>
Cc: <linux-kernel@vger.kernel.org>
Cc: Sascha Hauer <kernel@pengutronix.de>
Cc: <linux-arm-kernel@lists.infradead.org>
Cc: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
Signed-off-by: Benoît Thébaudeau <benoit.thebaudeau@advansee.com>
---
.../drivers/pwm/pwm-imx.c | 55 +++++++++++++++-----
1 file changed, 43 insertions(+), 12 deletions(-)
diff --git linux-next-c94456b.orig/drivers/pwm/pwm-imx.c linux-next-c94456b/drivers/pwm/pwm-imx.c
index 2a0b353..0519bf2 100644
--- linux-next-c94456b.orig/drivers/pwm/pwm-imx.c
+++ linux-next-c94456b/drivers/pwm/pwm-imx.c
@@ -55,6 +55,16 @@ static int imx_pwm_config(struct pwm_chip *chip,
{
struct imx_chip *imx = to_imx_chip(chip);
+ /*
+ * If the PWM is disabled, make sure to turn on the clock before
+ * accessing the registers.
+ */
+ if (!imx->clk_enabled) {
+ int rc = clk_prepare_enable(imx->clk);
+ if (rc)
+ return rc;
+ }
+
if (!(cpu_is_mx1() || cpu_is_mx21())) {
unsigned long long c;
unsigned long period_cycles, duty_cycles, prescale;
@@ -85,8 +95,11 @@ static int imx_pwm_config(struct pwm_chip *chip,
writel(period_cycles, imx->mmio_base + MX3_PWMPR);
cr = MX3_PWMCR_PRESCALER(prescale) |
- MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN |
- MX3_PWMCR_DBGEN | MX3_PWMCR_EN;
+ MX3_PWMCR_DOZEEN | MX3_PWMCR_WAITEN | MX3_PWMCR_DBGEN;
+
+ /* If the PWM is enabled, keep it so. */
+ if (imx->clk_enabled)
+ cr |= MX3_PWMCR_EN;
if (cpu_is_mx25())
cr |= MX3_PWMCR_CLKSRC_IPG;
@@ -118,32 +131,50 @@ static int imx_pwm_config(struct pwm_chip *chip,
BUG();
}
+ /* If the PWM is disabled, turn the clock off again to save power. */
+ if (!imx->clk_enabled)
+ clk_disable_unprepare(imx->clk);
+
return 0;
}
static int imx_pwm_enable(struct pwm_chip *chip, struct pwm_device *pwm)
{
struct imx_chip *imx = to_imx_chip(chip);
- int rc = 0;
+ int rc;
- if (!imx->clk_enabled) {
- rc = clk_prepare_enable(imx->clk);
- if (!rc)
- imx->clk_enabled = 1;
+ if (imx->clk_enabled)
+ return 0;
+
+ rc = clk_prepare_enable(imx->clk);
+ if (rc)
+ return rc;
+
+ if (!(cpu_is_mx1() || cpu_is_mx21())) {
+ u32 cr = readl(imx->mmio_base + MX3_PWMCR);
+ cr |= MX3_PWMCR_EN;
+ writel(cr, imx->mmio_base + MX3_PWMCR);
}
- return rc;
+
+ imx->clk_enabled = 1;
+ return 0;
}
static void imx_pwm_disable(struct pwm_chip *chip, struct pwm_device *pwm)
{
struct imx_chip *imx = to_imx_chip(chip);
- writel(0, imx->mmio_base + MX3_PWMCR);
+ if (!imx->clk_enabled)
+ return;
- if (imx->clk_enabled) {
- clk_disable_unprepare(imx->clk);
- imx->clk_enabled = 0;
+ if (!(cpu_is_mx1() || cpu_is_mx21())) {
+ u32 cr = readl(imx->mmio_base + MX3_PWMCR);
+ cr &= ~MX3_PWMCR_EN;
+ writel(cr, imx->mmio_base + MX3_PWMCR);
}
+
+ clk_disable_unprepare(imx->clk);
+ imx->clk_enabled = 0;
}
static struct pwm_ops imx_pwm_ops = {
^ permalink raw reply related
* Re: [PATCH 2/2] video: exynos_dp: move setting analog parameter and interrupt to after sw reset
From: Florian Tobias Schandinat @ 2012-08-23 20:50 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <000b01cd811d$c5890b60$509b2220$%han@samsung.com>
On 08/23/2012 10:55 AM, Jingoo Han wrote:
> SW reset sets DP TX to initial value, so configurations for analog parameter
> and interrupt are not set properly. Therefore, exynos_dp_init_analog_param()
> and exynos_dp_init_interrupt() should be moved to after sw reset is called,
> in order to set these values properly.
>
> Signed-off-by: Jingoo Han <jg1.han@samsung.com>
Applied.
Thanks,
Florian Tobias Schandinat
> ---
> drivers/video/exynos/exynos_dp_core.c | 3 +++
> drivers/video/exynos/exynos_dp_reg.c | 3 ---
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
> index c6c016a..8113698 100644
> --- a/drivers/video/exynos/exynos_dp_core.c
> +++ b/drivers/video/exynos/exynos_dp_core.c
> @@ -29,6 +29,9 @@ static int exynos_dp_init_dp(struct exynos_dp_device *dp)
>
> exynos_dp_swreset(dp);
>
> + exynos_dp_init_analog_param(dp);
> + exynos_dp_init_interrupt(dp);
> +
> /* SW defined function Normal operation */
> exynos_dp_enable_sw_function(dp);
>
> diff --git a/drivers/video/exynos/exynos_dp_reg.c b/drivers/video/exynos/exynos_dp_reg.c
> index e29497b..9a862f5 100644
> --- a/drivers/video/exynos/exynos_dp_reg.c
> +++ b/drivers/video/exynos/exynos_dp_reg.c
> @@ -148,9 +148,6 @@ void exynos_dp_reset(struct exynos_dp_device *dp)
> writel(0x2, dp->reg_base + EXYNOS_DP_M_AUD_GEN_FILTER_TH);
>
> writel(0x00000101, dp->reg_base + EXYNOS_DP_SOC_GENERAL_CTL);
> -
> - exynos_dp_init_analog_param(dp);
> - exynos_dp_init_interrupt(dp);
> }
>
> void exynos_dp_swreset(struct exynos_dp_device *dp)
^ permalink raw reply
* Re: [PATCH 1/2] video: exynos_dp: change return type of exynos_dp_init_video to void
From: Florian Tobias Schandinat @ 2012-08-23 20:49 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <000a01cd811d$a408b080$ec1a1180$%han@samsung.com>
On 08/23/2012 10:54 AM, Jingoo Han wrote:
> This patch changes return type of exynos_dp_init_video to void,
> because the return value is unnecessary.
>
> Signed-off-by: Jingoo Han <jg1.han@samsung.com>
Applied.
Thanks,
Florian Tobias Schandinat
> ---
> drivers/video/exynos/exynos_dp_core.h | 2 +-
> drivers/video/exynos/exynos_dp_reg.c | 4 +---
> 2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/video/exynos/exynos_dp_core.h b/drivers/video/exynos/exynos_dp_core.h
> index 8526e54..1e244ea 100644
> --- a/drivers/video/exynos/exynos_dp_core.h
> +++ b/drivers/video/exynos/exynos_dp_core.h
> @@ -105,7 +105,7 @@ u32 exynos_dp_get_lane1_link_training(struct exynos_dp_device *dp);
> u32 exynos_dp_get_lane2_link_training(struct exynos_dp_device *dp);
> u32 exynos_dp_get_lane3_link_training(struct exynos_dp_device *dp);
> void exynos_dp_reset_macro(struct exynos_dp_device *dp);
> -int exynos_dp_init_video(struct exynos_dp_device *dp);
> +void exynos_dp_init_video(struct exynos_dp_device *dp);
>
> void exynos_dp_set_video_color_format(struct exynos_dp_device *dp,
> u32 color_depth,
> diff --git a/drivers/video/exynos/exynos_dp_reg.c b/drivers/video/exynos/exynos_dp_reg.c
> index 2db5b9a..e29497b 100644
> --- a/drivers/video/exynos/exynos_dp_reg.c
> +++ b/drivers/video/exynos/exynos_dp_reg.c
> @@ -994,7 +994,7 @@ void exynos_dp_reset_macro(struct exynos_dp_device *dp)
> writel(reg, dp->reg_base + EXYNOS_DP_PHY_TEST);
> }
>
> -int exynos_dp_init_video(struct exynos_dp_device *dp)
> +void exynos_dp_init_video(struct exynos_dp_device *dp)
> {
> u32 reg;
>
> @@ -1012,8 +1012,6 @@ int exynos_dp_init_video(struct exynos_dp_device *dp)
>
> reg = VID_HRES_TH(2) | VID_VRES_TH(0);
> writel(reg, dp->reg_base + EXYNOS_DP_VIDEO_CTL_8);
> -
> - return 0;
> }
>
> void exynos_dp_set_video_color_format(struct exynos_dp_device *dp,
^ permalink raw reply
* Re: [PATCH 06/10] video: exynos_dp: Fix get_pll_lock_status return value
From: Florian Tobias Schandinat @ 2012-08-23 20:49 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1344398064-13563-7-git-send-email-seanpaul@chromium.org>
On 08/20/2012 09:20 AM, Jingoo Han wrote:
> On Wednesday, August 08, 2012 12:54 PM Sean Paul wrote:
>>
>> Fix the return value of exynos_dp_get_pll_lock_status to
>> reflect what it actually returns.
>>
>> Signed-off-by: Sean Paul <seanpaul@chromium.org>
>> Reviewed-by: Olof Johansson <olofj@chromium.org>
>
>
> Acked-by: Jingoo Han <jg1.han@samsung.com>
Applied.
Thanks,
Florian Tobias Schandinat
>
>
>> ---
>> drivers/video/exynos/exynos_dp_core.h | 2 +-
>> drivers/video/exynos/exynos_dp_reg.c | 2 +-
>> 2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/video/exynos/exynos_dp_core.h b/drivers/video/exynos/exynos_dp_core.h
>> index 8526e54..6431c65 100644
>> --- a/drivers/video/exynos/exynos_dp_core.h
>> +++ b/drivers/video/exynos/exynos_dp_core.h
>> @@ -43,7 +43,7 @@ void exynos_dp_init_interrupt(struct exynos_dp_device *dp);
>> void exynos_dp_reset(struct exynos_dp_device *dp);
>> void exynos_dp_swreset(struct exynos_dp_device *dp);
>> void exynos_dp_config_interrupt(struct exynos_dp_device *dp);
>> -u32 exynos_dp_get_pll_lock_status(struct exynos_dp_device *dp);
>> +enum pll_status exynos_dp_get_pll_lock_status(struct exynos_dp_device *dp);
>> void exynos_dp_set_pll_power_down(struct exynos_dp_device *dp, bool enable);
>> void exynos_dp_set_analog_power_down(struct exynos_dp_device *dp,
>> enum analog_power_block block,
>> diff --git a/drivers/video/exynos/exynos_dp_reg.c b/drivers/video/exynos/exynos_dp_reg.c
>> index a121bed..d7b1494 100644
>> --- a/drivers/video/exynos/exynos_dp_reg.c
>> +++ b/drivers/video/exynos/exynos_dp_reg.c
>> @@ -179,7 +179,7 @@ void exynos_dp_config_interrupt(struct exynos_dp_device *dp)
>> writel(reg, dp->reg_base + EXYNOS_DP_INT_STA_MASK);
>> }
>>
>> -u32 exynos_dp_get_pll_lock_status(struct exynos_dp_device *dp)
>> +enum pll_status exynos_dp_get_pll_lock_status(struct exynos_dp_device *dp)
>> {
>> u32 reg;
>>
>> --
>> 1.7.7.3
>
>
^ permalink raw reply
* Re: [PATCH 01/10] video: exynos_dp: Change aux transaction failures
From: Florian Tobias Schandinat @ 2012-08-23 20:48 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1344398064-13563-2-git-send-email-seanpaul@chromium.org>
On 08/20/2012 09:02 AM, Jingoo Han wrote:
> On Wednesday, August 08, 2012 12:54 PM Sean Paul wrote:
>>
>> This patch adds the function name to aux transaction failure messages
>> so we can tell which transaction is failing. It also changes the level
>> of Aux Transaction fail messages from error to debug. We retry the
>> transactions a few times and will report errors if warranted outside of
>> this function.
>>
>> Signed-off-by: Sean Paul <seanpaul@chromium.org>
>> Reviewed-by: Doug Anderson <dianders@chromium.org>
>> Reviewed-by: Bernie Thompson <bhthompson@chromium.org>
>> ---
>
>
> Acked-by: Jingoo Han <jg1.han@samsung.com>
>
> It looks good.
Applied.
Thanks,
Florian Tobias Schandinat
>
>
>
>> drivers/video/exynos/exynos_dp_reg.c | 21 ++++++++++++++-------
>> 1 files changed, 14 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/video/exynos/exynos_dp_reg.c b/drivers/video/exynos/exynos_dp_reg.c
>> index ce401c8..a121bed 100644
>> --- a/drivers/video/exynos/exynos_dp_reg.c
>> +++ b/drivers/video/exynos/exynos_dp_reg.c
>> @@ -471,7 +471,8 @@ int exynos_dp_write_byte_to_dpcd(struct exynos_dp_device *dp,
>> if (retval = 0)
>> break;
>> else
>> - dev_err(dp->dev, "Aux Transaction fail!\n");
>> + dev_dbg(dp->dev, "%s: Aux Transaction fail!\n",
>> + __func__);
>> }
>>
>> return retval;
>> @@ -511,7 +512,8 @@ int exynos_dp_read_byte_from_dpcd(struct exynos_dp_device *dp,
>> if (retval = 0)
>> break;
>> else
>> - dev_err(dp->dev, "Aux Transaction fail!\n");
>> + dev_dbg(dp->dev, "%s: Aux Transaction fail!\n",
>> + __func__);
>> }
>>
>> /* Read data buffer */
>> @@ -575,7 +577,8 @@ int exynos_dp_write_bytes_to_dpcd(struct exynos_dp_device *dp,
>> if (retval = 0)
>> break;
>> else
>> - dev_err(dp->dev, "Aux Transaction fail!\n");
>> + dev_dbg(dp->dev, "%s: Aux Transaction fail!\n",
>> + __func__);
>> }
>>
>> start_offset += cur_data_count;
>> @@ -632,7 +635,8 @@ int exynos_dp_read_bytes_from_dpcd(struct exynos_dp_device *dp,
>> if (retval = 0)
>> break;
>> else
>> - dev_err(dp->dev, "Aux Transaction fail!\n");
>> + dev_dbg(dp->dev, "%s: Aux Transaction fail!\n",
>> + __func__);
>> }
>>
>> for (cur_data_idx = 0; cur_data_idx < cur_data_count;
>> @@ -677,7 +681,7 @@ int exynos_dp_select_i2c_device(struct exynos_dp_device *dp,
>> /* Start AUX transaction */
>> retval = exynos_dp_start_aux_transaction(dp);
>> if (retval != 0)
>> - dev_err(dp->dev, "Aux Transaction fail!\n");
>> + dev_dbg(dp->dev, "%s: Aux Transaction fail!\n", __func__);
>>
>> return retval;
>> }
>> @@ -717,7 +721,8 @@ int exynos_dp_read_byte_from_i2c(struct exynos_dp_device *dp,
>> if (retval = 0)
>> break;
>> else
>> - dev_err(dp->dev, "Aux Transaction fail!\n");
>> + dev_dbg(dp->dev, "%s: Aux Transaction fail!\n",
>> + __func__);
>> }
>>
>> /* Read data */
>> @@ -777,7 +782,9 @@ int exynos_dp_read_bytes_from_i2c(struct exynos_dp_device *dp,
>> if (retval = 0)
>> break;
>> else
>> - dev_err(dp->dev, "Aux Transaction fail!\n");
>> + dev_dbg(dp->dev,
>> + "%s: Aux Transaction fail!\n",
>> + __func__);
>> }
>> /* Check if Rx sends defer */
>> reg = readl(dp->reg_base + EXYNOS_DP_AUX_RX_COMM);
>> --
>> 1.7.7.3
>
>
^ permalink raw reply
* Re: [PATCH 8/14] drivers/video/sunxvr2500.c: fix error return code
From: Florian Tobias Schandinat @ 2012-08-23 20:48 UTC (permalink / raw)
To: Julia Lawall; +Cc: kernel-janitors, linux-fbdev, linux-kernel
In-Reply-To: <1345365870-29831-9-git-send-email-Julia.Lawall@lip6.fr>
On 08/19/2012 08:44 AM, Julia Lawall wrote:
> From: Julia Lawall <Julia.Lawall@lip6.fr>
>
> Initialize return variable before exiting on an error path.
>
> A simplified version of the semantic match that finds this problem is as
> follows: (http://coccinelle.lip6.fr/)
>
> // <smpl>
> (
> if@p1 (\(ret < 0\|ret != 0\))
> { ... return ret; }
> |
> ret@p1 = 0
> )
> ... when != ret = e1
> when != &ret
> *if(...)
> {
> ... when != ret = e2
> when forall
> return ret;
> }
>
> // </smpl>
>
> Signed-off-by: Julia Lawall <Julia.Lawall@lip6.fr>
Applied.
Thanks,
Florian Tobias Schandinat
>
> ---
> drivers/video/sunxvr2500.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/video/sunxvr2500.c b/drivers/video/sunxvr2500.c
> index 5848436..7fbcba8 100644
> --- a/drivers/video/sunxvr2500.c
> +++ b/drivers/video/sunxvr2500.c
> @@ -181,8 +181,10 @@ static int __devinit s3d_pci_register(struct pci_dev *pdev,
> sp->fb_size = info->fix.line_length * sp->height;
>
> sp->fb_base = ioremap(sp->fb_base_phys, sp->fb_size);
> - if (!sp->fb_base)
> + if (!sp->fb_base) {
> + err = -ENOMEM;
> goto err_release_pci;
> + }
>
> err = s3d_set_fbinfo(sp);
> if (err)
>
>
^ permalink raw reply
* Re: [PATCH] fbdev/amifb: Remove write-only variable amifb_inverse
From: Florian Tobias Schandinat @ 2012-08-23 20:47 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: linux-fbdev, linux-m68k
In-Reply-To: <1345296621-15367-1-git-send-email-geert@linux-m68k.org>
On 08/18/2012 01:30 PM, Geert Uytterhoeven wrote:
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
Applied.
Thanks,
Florian Tobias Schandinat
> ---
> drivers/video/amifb.c | 2 --
> 1 files changed, 0 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/amifb.c b/drivers/video/amifb.c
> index 887df9d..7fa1bf8 100644
> --- a/drivers/video/amifb.c
> +++ b/drivers/video/amifb.c
> @@ -949,7 +949,6 @@ static int round_down_bpp = 1; /* for mode probing */
>
>
> static int amifb_ilbm = 0; /* interleaved or normal bitplanes */
> -static int amifb_inverse = 0;
>
> static u32 amifb_hfmin __initdata; /* monitor hfreq lower limit (Hz) */
> static u32 amifb_hfmax __initdata; /* monitor hfreq upper limit (Hz) */
> @@ -2355,7 +2354,6 @@ static int __init amifb_setup(char *options)
> if (!*this_opt)
> continue;
> if (!strcmp(this_opt, "inverse")) {
> - amifb_inverse = 1;
> fb_invert_cmaps();
> } else if (!strcmp(this_opt, "ilbm"))
> amifb_ilbm = 1;
^ permalink raw reply
* Re: [PATCH v5] da8xx-fb: add 24bpp LCD configuration support
From: Florian Tobias Schandinat @ 2012-08-23 20:47 UTC (permalink / raw)
To: linux-fbdev
In-Reply-To: <1344950502-18835-1-git-send-email-prakash.pm@ti.com>
On 08/14/2012 01:21 PM, Manjunathappa, Prakash wrote:
> LCD controller on am335x supports 24bpp raster configuration in addition
> to ones on da850. LCDC also supports 24bpp in unpacked format having
> ARGB:8888 32bpp format data in DDR, but it doesn't interpret alpha
> component of the data.
>
> Signed-off-by: Manjunathappa, Prakash <prakash.pm@ti.com>
> Cc: Anatolij Gustschin <agust@denx.de>
Applied, although I think it shouldn't set var->transp if the
transparency is not used. But as other drivers seem to do the same I
guess both is okay.
Thanks,
Florian Tobias Schandinat
> ---
> Applies on top of fbdev-next of Florian Tobias Schandinat's tree.
> Since v4:
> Re-define CNVT_TOHW macro.
> Since v3:
> Minor nit, declare pseudo_palette as u32 type.
> Since v2:
> Fixed additional configurations for 24bpp support.
> Since v1:
> Simplified calculation of pseudopalette for FB_VISUAL_TRUECOLOR type.
>
> drivers/video/da8xx-fb.c | 132 +++++++++++++++++++++++++++++++++------------
> 1 files changed, 97 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/video/da8xx-fb.c b/drivers/video/da8xx-fb.c
> index cb696ff..5c6df7b 100644
> --- a/drivers/video/da8xx-fb.c
> +++ b/drivers/video/da8xx-fb.c
> @@ -87,6 +87,8 @@
> #define LCD_V2_LIDD_CLK_EN BIT(1)
> #define LCD_V2_CORE_CLK_EN BIT(0)
> #define LCD_V2_LPP_B10 26
> +#define LCD_V2_TFT_24BPP_MODE BIT(25)
> +#define LCD_V2_TFT_24BPP_UNPACK BIT(26)
>
> /* LCD Raster Timing 2 Register */
> #define LCD_AC_BIAS_TRANSITIONS_PER_INT(x) ((x) << 16)
> @@ -157,7 +159,6 @@ struct da8xx_fb_par {
> unsigned int dma_end;
> struct clk *lcdc_clk;
> int irq;
> - unsigned short pseudo_palette[16];
> unsigned int palette_sz;
> unsigned int pxl_clk;
> int blank;
> @@ -176,6 +177,7 @@ struct da8xx_fb_par {
> unsigned int lcd_fck_rate;
> #endif
> void (*panel_power_ctrl)(int);
> + u32 pseudo_palette[16];
> };
>
> /* Variable Screen Information */
> @@ -528,6 +530,9 @@ static int lcd_cfg_frame_buffer(struct da8xx_fb_par *par, u32 width, u32 height,
> {
> u32 reg;
>
> + if (bpp > 16 && lcd_revision = LCD_VERSION_1)
> + return -EINVAL;
> +
> /* Set the Panel Width */
> /* Pixels per line = (PPL + 1)*16 */
> if (lcd_revision = LCD_VERSION_1) {
> @@ -571,14 +576,19 @@ static int lcd_cfg_frame_buffer(struct da8xx_fb_par *par, u32 width, u32 height,
> reg = lcdc_read(LCD_RASTER_CTRL_REG) & ~(1 << 8);
> if (raster_order)
> reg |= LCD_RASTER_ORDER;
> - lcdc_write(reg, LCD_RASTER_CTRL_REG);
> +
> + par->palette_sz = 16 * 2;
>
> switch (bpp) {
> case 1:
> case 2:
> case 4:
> case 16:
> - par->palette_sz = 16 * 2;
> + break;
> + case 24:
> + reg |= LCD_V2_TFT_24BPP_MODE;
> + case 32:
> + reg |= LCD_V2_TFT_24BPP_UNPACK;
> break;
>
> case 8:
> @@ -589,9 +599,12 @@ static int lcd_cfg_frame_buffer(struct da8xx_fb_par *par, u32 width, u32 height,
> return -EINVAL;
> }
>
> + lcdc_write(reg, LCD_RASTER_CTRL_REG);
> +
> return 0;
> }
>
> +#define CNVT_TOHW(val, width) ((((val) << (width)) + 0x7FFF - (val)) >> 16)
> static int fb_setcolreg(unsigned regno, unsigned red, unsigned green,
> unsigned blue, unsigned transp,
> struct fb_info *info)
> @@ -607,13 +620,38 @@ static int fb_setcolreg(unsigned regno, unsigned red, unsigned green,
> if (info->fix.visual = FB_VISUAL_DIRECTCOLOR)
> return 1;
>
> - if (info->var.bits_per_pixel = 4) {
> - if (regno > 15)
> - return 1;
> + if (info->var.bits_per_pixel > 16 && lcd_revision = LCD_VERSION_1)
> + return -EINVAL;
>
> - if (info->var.grayscale) {
> - pal = regno;
> - } else {
> + switch (info->fix.visual) {
> + case FB_VISUAL_TRUECOLOR:
> + red = CNVT_TOHW(red, info->var.red.length);
> + green = CNVT_TOHW(green, info->var.green.length);
> + blue = CNVT_TOHW(blue, info->var.blue.length);
> + break;
> + case FB_VISUAL_PSEUDOCOLOR:
> + switch (info->var.bits_per_pixel) {
> + case 4:
> + if (regno > 15)
> + return -EINVAL;
> +
> + if (info->var.grayscale) {
> + pal = regno;
> + } else {
> + red >>= 4;
> + green >>= 8;
> + blue >>= 12;
> +
> + pal = red & 0x0f00;
> + pal |= green & 0x00f0;
> + pal |= blue & 0x000f;
> + }
> + if (regno = 0)
> + pal |= 0x2000;
> + palette[regno] = pal;
> + break;
> +
> + case 8:
> red >>= 4;
> green >>= 8;
> blue >>= 12;
> @@ -621,36 +659,36 @@ static int fb_setcolreg(unsigned regno, unsigned red, unsigned green,
> pal = (red & 0x0f00);
> pal |= (green & 0x00f0);
> pal |= (blue & 0x000f);
> - }
> - if (regno = 0)
> - pal |= 0x2000;
> - palette[regno] = pal;
>
> - } else if (info->var.bits_per_pixel = 8) {
> - red >>= 4;
> - green >>= 8;
> - blue >>= 12;
> -
> - pal = (red & 0x0f00);
> - pal |= (green & 0x00f0);
> - pal |= (blue & 0x000f);
> -
> - if (palette[regno] != pal) {
> - update_hw = 1;
> - palette[regno] = pal;
> + if (palette[regno] != pal) {
> + update_hw = 1;
> + palette[regno] = pal;
> + }
> + break;
> }
> - } else if ((info->var.bits_per_pixel = 16) && regno < 16) {
> - red >>= (16 - info->var.red.length);
> - red <<= info->var.red.offset;
> + break;
> + }
>
> - green >>= (16 - info->var.green.length);
> - green <<= info->var.green.offset;
> + /* Truecolor has hardware independent palette */
> + if (info->fix.visual = FB_VISUAL_TRUECOLOR) {
> + u32 v;
>
> - blue >>= (16 - info->var.blue.length);
> - blue <<= info->var.blue.offset;
> + if (regno > 15)
> + return -EINVAL;
>
> - par->pseudo_palette[regno] = red | green | blue;
> + v = (red << info->var.red.offset) |
> + (green << info->var.green.offset) |
> + (blue << info->var.blue.offset);
>
> + switch (info->var.bits_per_pixel) {
> + case 16:
> + ((u16 *) (info->pseudo_palette))[regno] = v;
> + break;
> + case 24:
> + case 32:
> + ((u32 *) (info->pseudo_palette))[regno] = v;
> + break;
> + }
> if (palette[0] != 0x4000) {
> update_hw = 1;
> palette[0] = 0x4000;
> @@ -663,6 +701,7 @@ static int fb_setcolreg(unsigned regno, unsigned red, unsigned green,
>
> return 0;
> }
> +#undef CNVT_TOHW
>
> static void lcd_reset(struct da8xx_fb_par *par)
> {
> @@ -871,6 +910,9 @@ static int fb_check_var(struct fb_var_screeninfo *var,
> {
> int err = 0;
>
> + if (var->bits_per_pixel > 16 && lcd_revision = LCD_VERSION_1)
> + return -EINVAL;
> +
> switch (var->bits_per_pixel) {
> case 1:
> case 8:
> @@ -906,6 +948,26 @@ static int fb_check_var(struct fb_var_screeninfo *var,
> var->transp.length = 0;
> var->nonstd = 0;
> break;
> + case 24:
> + var->red.offset = 16;
> + var->red.length = 8;
> + var->green.offset = 8;
> + var->green.length = 8;
> + var->blue.offset = 0;
> + var->blue.length = 8;
> + var->nonstd = 0;
> + break;
> + case 32:
> + var->transp.offset = 24;
> + var->transp.length = 8;
> + var->red.offset = 16;
> + var->red.length = 8;
> + var->green.offset = 8;
> + var->green.length = 8;
> + var->blue.offset = 0;
> + var->blue.length = 8;
> + var->nonstd = 0;
> + break;
> default:
> err = -EINVAL;
> }
^ permalink raw reply
* Re: [PATCH] video:uvesafb: check the return value of kzalloc
From: Florian Tobias Schandinat @ 2012-08-23 20:43 UTC (permalink / raw)
To: Wang YanQing, linux-fbdev, linux-kernel, spock
In-Reply-To: <20120813100232.GA16363@udknight>
On 08/13/2012 10:02 AM, Wang YanQing wrote:
>
> Michal maybe forgot it merely, we should add code
> to check the return value of kzalloc to make the
> code more robust.
>
> Signed-off-by: Wang YanQing <udknight@gmail.com>
Applied.
Thanks,
Florian Tobias Schandinat
> ---
> drivers/video/uvesafb.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/video/uvesafb.c b/drivers/video/uvesafb.c
> index b0e2a42..2f8f82d 100644
> --- a/drivers/video/uvesafb.c
> +++ b/drivers/video/uvesafb.c
> @@ -659,6 +659,8 @@ static int __devinit uvesafb_vbe_getedid(struct uvesafb_ktask *task,
> task->t.flags = TF_BUF_RET | TF_BUF_ESDI;
> task->t.buf_len = EDID_LENGTH;
> task->buf = kzalloc(EDID_LENGTH, GFP_KERNEL);
> + if (!task->buf)
> + return -ENOMEM;
>
> err = uvesafb_exec(task);
>
^ 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