fbdev: SH-Mobile MIPI-DSI Update
[PATCH 01/05] fbdev: sh_mipi_dsi: Remove request/release mem region
[PATCH 02/05] fbdev: sh_mipi_dsi: Make use of register names
[PATCH 03/05] ARM: mach-shmobile: Extend AP4EVB MIPI-DSI resources
[PATCH 04/05] fbdev: sh_mipi_dsi: Require two I/O resources
[PATCH 05/05] fbdev: sh_mipi_dsi: Allow LCDC board callbacks
These patches contain a few rather trivial updates for the
sh_mipi_dsi driver. Necessary changes to the AP4EVB specific
platform data is also included in [PATCH 03/05]. It should
be applied before [PATCH 04/05].
The patches above contain the following two major features:
- Support newer SoCs with different I/O base adresses
- Allow board-specific LCDC hooks together with MIPI-DSI
Signed-off-by: Magnus Damm <[email protected]>
---
arch/arm/mach-shmobile/board-ap4evb.c | 7 +
drivers/video/sh_mipi_dsi.c | 131 ++++++++++++++++++++-------------
2 files changed, 87 insertions(+), 51 deletions(-)
From: Magnus Damm <[email protected]>
The driver core already manages resources for us, so
there is no need to perform request_mem_region() and
release_mem_region() in the MIPI-DSI driver.
Signed-off-by: Magnus Damm <[email protected]>
---
drivers/video/sh_mipi_dsi.c | 11 -----------
1 file changed, 11 deletions(-)
--- 0001/drivers/video/sh_mipi_dsi.c
+++ work/drivers/video/sh_mipi_dsi.c 2010-11-16 17:34:08.000000000 +0900
@@ -344,12 +344,6 @@ static int __init sh_mipi_probe(struct p
goto ealloc;
}
- if (!request_mem_region(res->start, resource_size(res), pdev->name)) {
- dev_err(&pdev->dev, "MIPI register region already claimed\n");
- ret = -EBUSY;
- goto ereqreg;
- }
-
mipi->base = ioremap(res->start, resource_size(res));
if (!mipi->base) {
ret = -ENOMEM;
@@ -433,8 +427,6 @@ esettrate:
eclktget:
iounmap(mipi->base);
emap:
- release_mem_region(res->start, resource_size(res));
-ereqreg:
kfree(mipi);
ealloc:
efindslot:
@@ -446,7 +438,6 @@ efindslot:
static int __exit sh_mipi_remove(struct platform_device *pdev)
{
struct sh_mipi_dsi_info *pdata = pdev->dev.platform_data;
- struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
struct sh_mipi *mipi = platform_get_drvdata(pdev);
int i, ret;
@@ -476,8 +467,6 @@ static int __exit sh_mipi_remove(struct
clk_put(mipi->dsit_clk);
clk_put(mipi->dsip_clk);
iounmap(mipi->base);
- if (res)
- release_mem_region(res->start, resource_size(res));
platform_set_drvdata(pdev, NULL);
kfree(mipi);
From: Magnus Damm <[email protected]>
Keep MIPI-DSI registers in one place instead of
using magic values together with comments.
Signed-off-by: Magnus Damm <[email protected]>
---
drivers/video/sh_mipi_dsi.c | 62 +++++++++++++++++++++++++++----------------
1 file changed, 39 insertions(+), 23 deletions(-)
--- 0005/drivers/video/sh_mipi_dsi.c
+++ work/drivers/video/sh_mipi_dsi.c 2010-11-16 16:48:37.000000000 +0900
@@ -21,10 +21,26 @@
#include <video/sh_mipi_dsi.h>
#include <video/sh_mobile_lcdc.h>
-#define CMTSRTCTR 0x80d0
+#define SYSCTRL 0x0000
+#define SYSCONF 0x0004
+#define TIMSET 0x0008
+#define RESREQSET0 0x0018
+#define RESREQSET1 0x001c
+#define HSTTOVSET 0x0020
+#define LPRTOVSET 0x0024
+#define TATOVSET 0x0028
+#define PRTOVSET 0x002c
+#define DSICTRL 0x0030
+#define DSIINTE 0x0060
+#define PHYCTRL 0x0070
+
+#define DTCTR 0x8000
+#define VMCTR1 0x8020
+#define VMCTR2 0x8024
+#define VMLEN1 0x8028
#define CMTSRTREQ 0x8070
+#define CMTSRTCTR 0x80d0
-#define DSIINTE 0x0060
/* E.g., sh7372 has 2 MIPI-DSIs - one for each LCDC */
#define MAX_SH_MIPI_DSI 2
@@ -55,10 +71,10 @@ static int sh_mipi_send_short(struct sh_
int cnt = 100;
/* transmit a short packet to LCD panel */
- iowrite32(1 | data, mipi->base + 0x80d0); /* CMTSRTCTR */
- iowrite32(1, mipi->base + 0x8070); /* CMTSRTREQ */
+ iowrite32(1 | data, mipi->base + CMTSRTCTR);
+ iowrite32(1, mipi->base + CMTSRTREQ);
- while ((ioread32(mipi->base + 0x8070) & 1) && --cnt)
+ while ((ioread32(mipi->base + CMTSRTREQ) & 1) && --cnt)
udelay(1);
return cnt ? 0 : -ETIMEDOUT;
@@ -90,7 +106,7 @@ static void sh_mipi_dsi_enable(struct sh
* enable LCDC data tx, transition to LPS after completion of each HS
* packet
*/
- iowrite32(0x00000002 | enable, mipi->base + 0x8000); /* DTCTR */
+ iowrite32(0x00000002 | enable, mipi->base + DTCTR);
}
static void sh_mipi_shutdown(struct platform_device *pdev)
@@ -223,10 +239,10 @@ static int __init sh_mipi_setup(struct s
return -EINVAL;
/* reset DSI link */
- iowrite32(0x00000001, base); /* SYSCTRL */
+ iowrite32(0x00000001, base + SYSCTRL);
/* Hold reset for 100 cycles of the slowest of bus, HS byte and LP clock */
udelay(50);
- iowrite32(0x00000000, base); /* SYSCTRL */
+ iowrite32(0x00000000, base + SYSCTRL);
/* setup DSI link */
@@ -238,7 +254,7 @@ static int __init sh_mipi_setup(struct s
* ECC check enable
* additionally enable first two lanes
*/
- iowrite32(0x00003703, base + 0x04); /* SYSCONF */
+ iowrite32(0x00003703, base + SYSCONF);
/*
* T_wakeup = 0x7000
* T_hs-trail = 3
@@ -246,28 +262,28 @@ static int __init sh_mipi_setup(struct s
* T_clk-trail = 3
* T_clk-prepare = 2
*/
- iowrite32(0x70003332, base + 0x08); /* TIMSET */
+ iowrite32(0x70003332, base + TIMSET);
/* no responses requested */
- iowrite32(0x00000000, base + 0x18); /* RESREQSET0 */
+ iowrite32(0x00000000, base + RESREQSET0);
/* request response to packets of type 0x28 */
- iowrite32(0x00000100, base + 0x1c); /* RESREQSET1 */
+ iowrite32(0x00000100, base + RESREQSET1);
/* High-speed transmission timeout, default 0xffffffff */
- iowrite32(0x0fffffff, base + 0x20); /* HSTTOVSET */
+ iowrite32(0x0fffffff, base + HSTTOVSET);
/* LP reception timeout, default 0xffffffff */
- iowrite32(0x0fffffff, base + 0x24); /* LPRTOVSET */
+ iowrite32(0x0fffffff, base + LPRTOVSET);
/* Turn-around timeout, default 0xffffffff */
- iowrite32(0x0fffffff, base + 0x28); /* TATOVSET */
+ iowrite32(0x0fffffff, base + TATOVSET);
/* Peripheral reset timeout, default 0xffffffff */
- iowrite32(0x0fffffff, base + 0x2c); /* PRTOVSET */
+ iowrite32(0x0fffffff, base + PRTOVSET);
/* Enable timeout counters */
- iowrite32(0x00000f00, base + 0x30); /* DSICTRL */
+ iowrite32(0x00000f00, base + DSICTRL);
/* Interrupts not used, disable all */
iowrite32(0, base + DSIINTE);
/* DSI-Tx bias on */
- iowrite32(0x00000001, base + 0x70); /* PHYCTRL */
+ iowrite32(0x00000001, base + PHYCTRL);
udelay(200);
/* Deassert resets, power on, set multiplier */
- iowrite32(0x03070b01, base + 0x70); /* PHYCTRL */
+ iowrite32(0x03070b01, base + PHYCTRL);
/* setup l-bridge */
@@ -275,20 +291,20 @@ static int __init sh_mipi_setup(struct s
* Enable transmission of all packets,
* transmit LPS after each HS packet completion
*/
- iowrite32(0x00000006, base + 0x8000); /* DTCTR */
+ iowrite32(0x00000006, base + DTCTR);
/* VSYNC width = 2 (<< 17) */
- iowrite32(0x00040000 | (pctype << 12) | datatype, base + 0x8020); /* VMCTR1 */
+ iowrite32(0x00040000 | (pctype << 12) | datatype, base + VMCTR1);
/*
* Non-burst mode with sync pulses: VSE and HSE are output,
* HSA period allowed, no commands in LP
*/
- iowrite32(0x00e00000, base + 0x8024); /* VMCTR2 */
+ iowrite32(0x00e00000, base + VMCTR2);
/*
* 0x660 = 1632 bytes per line (RGB24, 544 pixels: see
* sh_mobile_lcdc_info.ch[0].lcd_cfg[0].xres), HSALEN = 1 - default
* (unused, since VMCTR2[HSABM] = 0)
*/
- iowrite32(1 | (linelength << 16), base + 0x8028); /* VMLEN1 */
+ iowrite32(1 | (linelength << 16), base + VMLEN1);
msleep(5);
From: Magnus Damm <[email protected]>
Update the AP4EVB board code to go from a single I/O
resource to two I/O resources for the MIPI-DSI driver.
Signed-off-by: Magnus Damm <[email protected]>
---
arch/arm/mach-shmobile/board-ap4evb.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
--- 0001/arch/arm/mach-shmobile/board-ap4evb.c
+++ work/arch/arm/mach-shmobile/board-ap4evb.c 2010-11-16 17:19:30.000000000 +0900
@@ -501,7 +501,12 @@ static struct platform_device keysc_devi
static struct resource mipidsi0_resources[] = {
[0] = {
.start = 0xffc60000,
- .end = 0xffc68fff,
+ .end = 0xffc63073,
+ .flags = IORESOURCE_MEM,
+ },
+ [1] = {
+ .start = 0xffc68000,
+ .end = 0xffc680ef,
.flags = IORESOURCE_MEM,
},
};
From: Magnus Damm <[email protected]>
This patch updates the MIPI-DSI driver to use a
second I/O resource to specify the base address
for the link hardware block. The base address for
the link hardware block seems to vary with SoC
type. Using two I/O resources to describe the
MIPI-DSI hardware allows us to support both newer
and older SoCs.
Signed-off-by: Magnus Damm <[email protected]>
---
drivers/video/sh_mipi_dsi.c | 44 +++++++++++++++++++++++++++----------------
1 file changed, 28 insertions(+), 16 deletions(-)
--- 0006/drivers/video/sh_mipi_dsi.c
+++ work/drivers/video/sh_mipi_dsi.c 2010-11-16 17:30:10.000000000 +0900
@@ -34,19 +34,20 @@
#define DSIINTE 0x0060
#define PHYCTRL 0x0070
-#define DTCTR 0x8000
-#define VMCTR1 0x8020
-#define VMCTR2 0x8024
-#define VMLEN1 0x8028
-#define CMTSRTREQ 0x8070
-#define CMTSRTCTR 0x80d0
-
+/* relative to linkbase */
+#define DTCTR 0x0000
+#define VMCTR1 0x0020
+#define VMCTR2 0x0024
+#define VMLEN1 0x0028
+#define CMTSRTREQ 0x0070
+#define CMTSRTCTR 0x00d0
/* E.g., sh7372 has 2 MIPI-DSIs - one for each LCDC */
#define MAX_SH_MIPI_DSI 2
struct sh_mipi {
void __iomem *base;
+ void __iomem *linkbase;
struct clk *dsit_clk;
struct clk *dsip_clk;
};
@@ -71,10 +72,10 @@ static int sh_mipi_send_short(struct sh_
int cnt = 100;
/* transmit a short packet to LCD panel */
- iowrite32(1 | data, mipi->base + CMTSRTCTR);
- iowrite32(1, mipi->base + CMTSRTREQ);
+ iowrite32(1 | data, mipi->linkbase + CMTSRTCTR);
+ iowrite32(1, mipi->linkbase + CMTSRTREQ);
- while ((ioread32(mipi->base + CMTSRTREQ) & 1) && --cnt)
+ while ((ioread32(mipi->linkbase + CMTSRTREQ) & 1) && --cnt)
udelay(1);
return cnt ? 0 : -ETIMEDOUT;
@@ -106,7 +107,7 @@ static void sh_mipi_dsi_enable(struct sh
* enable LCDC data tx, transition to LPS after completion of each HS
* packet
*/
- iowrite32(0x00000002 | enable, mipi->base + DTCTR);
+ iowrite32(0x00000002 | enable, mipi->linkbase + DTCTR);
}
static void sh_mipi_shutdown(struct platform_device *pdev)
@@ -291,20 +292,21 @@ static int __init sh_mipi_setup(struct s
* Enable transmission of all packets,
* transmit LPS after each HS packet completion
*/
- iowrite32(0x00000006, base + DTCTR);
+ iowrite32(0x00000006, mipi->linkbase + DTCTR);
/* VSYNC width = 2 (<< 17) */
- iowrite32(0x00040000 | (pctype << 12) | datatype, base + VMCTR1);
+ iowrite32(0x00040000 | (pctype << 12) | datatype,
+ mipi->linkbase + VMCTR1);
/*
* Non-burst mode with sync pulses: VSE and HSE are output,
* HSA period allowed, no commands in LP
*/
- iowrite32(0x00e00000, base + VMCTR2);
+ iowrite32(0x00e00000, mipi->linkbase + VMCTR2);
/*
* 0x660 = 1632 bytes per line (RGB24, 544 pixels: see
* sh_mobile_lcdc_info.ch[0].lcd_cfg[0].xres), HSALEN = 1 - default
* (unused, since VMCTR2[HSABM] = 0)
*/
- iowrite32(1 | (linelength << 16), base + VMLEN1);
+ iowrite32(1 | (linelength << 16), mipi->linkbase + VMLEN1);
msleep(5);
@@ -337,11 +339,12 @@ static int __init sh_mipi_probe(struct p
struct sh_mipi *mipi;
struct sh_mipi_dsi_info *pdata = pdev->dev.platform_data;
struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ struct resource *res2 = platform_get_resource(pdev, IORESOURCE_MEM, 1);
unsigned long rate, f_current;
int idx = pdev->id, ret;
char dsip_clk[] = "dsi.p_clk";
- if (!res || idx >= ARRAY_SIZE(mipi_dsi) || !pdata)
+ if (!res || !res2 || idx >= ARRAY_SIZE(mipi_dsi) || !pdata)
return -ENODEV;
mutex_lock(&array_lock);
@@ -366,6 +369,12 @@ static int __init sh_mipi_probe(struct p
goto emap;
}
+ mipi->linkbase = ioremap(res2->start, resource_size(res2));
+ if (!mipi->linkbase) {
+ ret = -ENOMEM;
+ goto emap2;
+ }
+
mipi->dsit_clk = clk_get(&pdev->dev, "dsit_clk");
if (IS_ERR(mipi->dsit_clk)) {
ret = PTR_ERR(mipi->dsit_clk);
@@ -441,6 +450,8 @@ eclkpget:
esettrate:
clk_put(mipi->dsit_clk);
eclktget:
+ iounmap(mipi->linkbase);
+emap2:
iounmap(mipi->base);
emap:
kfree(mipi);
@@ -483,6 +494,7 @@ static int __exit sh_mipi_remove(struct
clk_disable(mipi->dsit_clk);
clk_put(mipi->dsit_clk);
clk_put(mipi->dsip_clk);
+ iounmap(mipi->linkbase);
iounmap(mipi->base);
platform_set_drvdata(pdev, NULL);
kfree(mipi);
From: Magnus Damm <[email protected]>
Update the MIPI-DSI driver to make use of the
LCD panel callbacks in the LCDC platform data.
Without this patch MIPI panels cannot use board
specific LCDC callbacks to control power and/or
back light.
Signed-off-by: Magnus Damm <[email protected]>
---
drivers/video/sh_mipi_dsi.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
--- 0008/drivers/video/sh_mipi_dsi.c
+++ work/drivers/video/sh_mipi_dsi.c 2010-11-16 17:41:32.000000000 +0900
@@ -50,6 +50,9 @@ struct sh_mipi {
void __iomem *linkbase;
struct clk *dsit_clk;
struct clk *dsip_clk;
+ void *next_board_data;
+ void (*next_display_on)(void *board_data, struct fb_info *info);
+ void (*next_display_off)(void *board_data);
};
static struct sh_mipi *mipi_dsi[MAX_SH_MIPI_DSI];
@@ -122,12 +125,18 @@ static void mipi_display_on(void *arg, s
struct sh_mipi *mipi = arg;
sh_mipi_dsi_enable(mipi, true);
+
+ if (mipi->next_display_on)
+ mipi->next_display_on(mipi->next_board_data, info);
}
static void mipi_display_off(void *arg)
{
struct sh_mipi *mipi = arg;
+ if (mipi->next_display_off)
+ mipi->next_display_off(mipi->next_board_data);
+
sh_mipi_dsi_enable(mipi, false);
}
@@ -431,6 +440,11 @@ static int __init sh_mipi_probe(struct p
mutex_unlock(&array_lock);
platform_set_drvdata(pdev, mipi);
+ /* Save original LCDC callbacks */
+ mipi->next_board_data = pdata->lcd_chan->board_cfg.board_data;
+ mipi->next_display_on = pdata->lcd_chan->board_cfg.display_on;
+ mipi->next_display_off = pdata->lcd_chan->board_cfg.display_off;
+
/* Set up LCDC callbacks */
pdata->lcd_chan->board_cfg.board_data = mipi;
pdata->lcd_chan->board_cfg.display_on = mipi_display_on;
On Wed, Nov 17, 2010 at 03:44:15PM +0900, Magnus Damm wrote:
> The driver core already manages resources for us, so
> there is no need to perform request_mem_region() and
> release_mem_region() in the MIPI-DSI driver.
>
No. This comes up time and time again for some reason, I'm not sure why
this misconception keeps being propagated (and it's definitely a mistake
I've made myself too!)
The driver core simply takes your resources and inserts them in to the
resource tree, it has nothing to do with requesting ranges, or checking
for conflicts. The main rationale for this is that drivers (think MFDs)
may pass in a large resource range, which in turn will be broken up and
requested by individual drivers nested underneath it. Only at request
time will the requested range (which can be the entire thing, or a
subset) be flagged as busy and conflicts detected.
The current logic is quite correct in what it is doing.
On Wed, 17 Nov 2010, Magnus Damm wrote:
> fbdev: SH-Mobile MIPI-DSI Update
>
> [PATCH 01/05] fbdev: sh_mipi_dsi: Remove request/release mem region
> [PATCH 02/05] fbdev: sh_mipi_dsi: Make use of register names
> [PATCH 03/05] ARM: mach-shmobile: Extend AP4EVB MIPI-DSI resources
> [PATCH 04/05] fbdev: sh_mipi_dsi: Require two I/O resources
> [PATCH 05/05] fbdev: sh_mipi_dsi: Allow LCDC board callbacks
Thanks for the patches, Magnus! Agree with Paul regarding [PATCH 1/5], for
2-5 here's an
Acked-by: Guennadi Liakhovetski <[email protected]>
Regards
Guennadi
>
> These patches contain a few rather trivial updates for the
> sh_mipi_dsi driver. Necessary changes to the AP4EVB specific
> platform data is also included in [PATCH 03/05]. It should
> be applied before [PATCH 04/05].
>
> The patches above contain the following two major features:
> - Support newer SoCs with different I/O base adresses
> - Allow board-specific LCDC hooks together with MIPI-DSI
>
> Signed-off-by: Magnus Damm <[email protected]>
> ---
>
> arch/arm/mach-shmobile/board-ap4evb.c | 7 +
> drivers/video/sh_mipi_dsi.c | 131 ++++++++++++++++++++-------------
> 2 files changed, 87 insertions(+), 51 deletions(-)
>
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
On Wed, Nov 17, 2010 at 4:25 PM, Paul Mundt <[email protected]> wrote:
> On Wed, Nov 17, 2010 at 03:44:15PM +0900, Magnus Damm wrote:
>> The driver core already manages resources for us, so
>> there is no need to perform request_mem_region() and
>> release_mem_region() in the MIPI-DSI driver.
>>
> No. This comes up time and time again for some reason, I'm not sure why
> this misconception keeps being propagated (and it's definitely a mistake
> I've made myself too!)
Perhaps the confusing part is the -EBUSY error case for
insert_resource() in platform_device_add() located in
drivers/base/platform. Judging by that code it sure looks like there
is some kind of conflict management, but probably not enough as you
mention below.
> The driver core simply takes your resources and inserts them in to the
> resource tree, it has nothing to do with requesting ranges, or checking
> for conflicts. The main rationale for this is that drivers (think MFDs)
> may pass in a large resource range, which in turn will be broken up and
> requested by individual drivers nested underneath it. Only at request
> time will the requested range (which can be the entire thing, or a
> subset) be flagged as busy and conflicts detected.
Right, I understand.
> The current logic is quite correct in what it is doing.
It may be correct, but I don't like the smell of it.
Sound like begging for trouble to me - to require the majority of the
drivers (the simple ones) to call request/release_mem_region() even
though by context it's quite obvious that the driver will use
resource. It doesn't make sense to pass resources that are not going
to be used, does it?
Also, there are quite a few levels of resource handling / conflict
checking / device access:
1) board/cpu code - contains struct resource for devices
2) platform_device_add() - insert_resource() does basic checks
3) driver probe() request_mem_region() does more checks, reserves
4) driver probe() ioremap() gets a virtual address range
5) driver uses ioreadN()/iowriteN() on virtual address range
The physical range provided by struct resource and the virtual address
range from ioremap() are of course different things, but i think there
is a bit of overlap there. The driver isn't supposed to be using
ioremap() on an I/O memory region that hasn't been locked down with
request_mem_region() for instance.
I'd like to see a simplified interface that handles
request_mem_region() and ioremap() in the driver code, perhaps
together with automatic handling of request_irq(). Perhaps that's
difficult in real life, but if possible then that would cut away quite
a bit of code.
And then there are device trees on top of all this, yay!
/ magnus
On Wed, Nov 17, 2010 at 05:51:41PM +0900, Magnus Damm wrote:
> On Wed, Nov 17, 2010 at 4:25 PM, Paul Mundt <[email protected]> wrote:
> > No. This comes up time and time again for some reason, I'm not sure why
> > this misconception keeps being propagated (and it's definitely a mistake
> > I've made myself too!)
>
> Perhaps the confusing part is the -EBUSY error case for
> insert_resource() in platform_device_add() located in
> drivers/base/platform. Judging by that code it sure looks like there
> is some kind of conflict management, but probably not enough as you
> mention below.
>
This is conflict management on a different level, and has nothing to do
with the in-use region case which is separately accounted. It will handle
duplicate insertion of the same resource, but that doesn't prevent
multiple users from getting a handle on the same memory within the
region. request_mem_region() and friends provide this explicitly.
> > The current logic is quite correct in what it is doing.
>
> It may be correct, but I don't like the smell of it.
>
> Sound like begging for trouble to me - to require the majority of the
> drivers (the simple ones) to call request/release_mem_region() even
> though by context it's quite obvious that the driver will use
> resource. It doesn't make sense to pass resources that are not going
> to be used, does it?
>
Begging for trouble is deleting the resource management and hoping for
the best. Plenty of drivers pass in a large resource and then carve it up
internally, it's only a detail the driver can work out for itself, and
this is not knowledge that can be asserted generically.
You seem to have selectively ignored the MFD example that I cited, but
there are plenty of non-MFD cases that do this as well. Some drivers do
follow the one resource per logical abstraction approach that you seem to
favour, mostly using named resources and groveling with
platform_get_resource_byname(), but this is hardly the only way to go
about it, and is by far the exception rather than the rule.
> Also, there are quite a few levels of resource handling / conflict
> checking / device access:
>
> 1) board/cpu code - contains struct resource for devices
> 2) platform_device_add() - insert_resource() does basic checks
> 3) driver probe() request_mem_region() does more checks, reserves
> 4) driver probe() ioremap() gets a virtual address range
> 5) driver uses ioreadN()/iowriteN() on virtual address range
>
And each one of these steps is completely orthogonal to the other, so
what exactly is your point? As it is, these all layer quite cleanly.
> The physical range provided by struct resource and the virtual address
> range from ioremap() are of course different things, but i think there
> is a bit of overlap there. The driver isn't supposed to be using
> ioremap() on an I/O memory region that hasn't been locked down with
> request_mem_region() for instance.
>
> I'd like to see a simplified interface that handles
> request_mem_region() and ioremap() in the driver code, perhaps
> together with automatic handling of request_irq(). Perhaps that's
> difficult in real life, but if possible then that would cut away quite
> a bit of code.
>
The IRQ thing is obviously wholly unrelated and so doesn't factor in, but
for the memory case you cite it sounds like you're after some devres and
general helpers akin to what the PCI code has going with things like
pci_request_regions() and the managed iomap helpers.
Things work reasonably well in PCI land since you have at least a fairly
logical and consistent abstraction in terms of how the BARs are arranged,
but even then the request/remap helpers take a BAR mask to work out which
ones to deal with in what way (your ioremap()'ed window will want to be
requested, but not all requested windows will want to be remapped). You
could probably do this for platform devices too if you really wanted to,
but you're not likely to be able to generalize much beyond that.
The end result however is that you're still required to deal with
flagging the region (or the parts of the region your driver is
immediately concerned with) as busy, so dropping it as you are doing here
is unacceptable regardless of whether you are using a helper to make life
easier or not.
On Wed, Nov 17, 2010 at 6:30 PM, Paul Mundt <[email protected]> wrote:
> On Wed, Nov 17, 2010 at 05:51:41PM +0900, Magnus Damm wrote:
>> On Wed, Nov 17, 2010 at 4:25 PM, Paul Mundt <[email protected]> wrote:
>> > No. This comes up time and time again for some reason, I'm not sure why
>> > this misconception keeps being propagated (and it's definitely a mistake
>> > I've made myself too!)
>>
>> Perhaps the confusing part is the -EBUSY error case for
>> insert_resource() in platform_device_add() located in
>> drivers/base/platform. Judging by that code it sure looks like there
>> is some kind of conflict management, but probably not enough as you
>> mention below.
>>
> This is conflict management on a different level, and has nothing to do
> with the in-use region case which is separately accounted. It will handle
> duplicate insertion of the same resource, but that doesn't prevent
> multiple users from getting a handle on the same memory within the
> region. request_mem_region() and friends provide this explicitly.
Sure, I'm with you on that.
>> > The current logic is quite correct in what it is doing.
>>
>> It may be correct, but I don't like the smell of it.
>>
>> Sound like begging for trouble to me - to require the majority of the
>> drivers (the simple ones) to call request/release_mem_region() even
>> though by context it's quite obvious that the driver will use
>> resource. It doesn't make sense to pass resources that are not going
>> to be used, does it?
>>
> Begging for trouble is deleting the resource management and hoping for
> the best. Plenty of drivers pass in a large resource and then carve it up
> internally, it's only a detail the driver can work out for itself, and
> this is not knowledge that can be asserted generically.
I'm not saying that you shouldn't do resource management, my point is
that asking each driver to do request_mem_region() is error prone and
it is (or should be) such a common operation that it may make sense to
have some helper code in place.
> You seem to have selectively ignored the MFD example that I cited, but
> there are plenty of non-MFD cases that do this as well. Some drivers do
> follow the one resource per logical abstraction approach that you seem to
> favour, mostly using named resources and groveling with
> platform_get_resource_byname(), but this is hardly the only way to go
> about it, and is by far the exception rather than the rule.
Uhm, for platform drivers only 6 out of 155 were located in
drivers/mfd, so I wouldn't call that very common. But of course, MFD
drivers need to be handled as well.
>> Also, there are quite a few levels of resource handling / conflict
>> checking / device access:
>>
>> 1) board/cpu code - contains struct resource for devices
>> 2) platform_device_add() - insert_resource() does basic checks
>> 3) driver probe() request_mem_region() does more checks, reserves
>> 4) driver probe() ioremap() gets a virtual address range
>> 5) driver uses ioreadN()/iowriteN() on virtual address range
>>
> And each one of these steps is completely orthogonal to the other, so
> what exactly is your point? As it is, these all layer quite cleanly.
Yes, it layers cleanly, but I suspect that a majority of all device
drivers never need this kind of flexibility.
>> The physical range provided by struct resource and the virtual address
>> range from ioremap() are of course different things, but i think there
>> is a bit of overlap there. The driver isn't supposed to be using
>> ioremap() on an I/O memory region that hasn't been locked down with
>> request_mem_region() for instance.
I believe that this overlap means that more optimal interfaces could
be used for a wide range of device drivers.
>> I'd like to see a simplified interface that handles
>> request_mem_region() and ioremap() in the driver code, perhaps
>> together with automatic handling of request_irq(). Perhaps that's
>> difficult in real life, but if possible then that would cut away quite
>> a bit of code.
>>
> The IRQ thing is obviously wholly unrelated and so doesn't factor in, but
> for the memory case you cite it sounds like you're after some devres and
> general helpers akin to what the PCI code has going with things like
> pci_request_regions() and the managed iomap helpers.
The IRQ thing may be unrelated, but I would prefer a higher level of
interrupt request function that operates on platform device or similar
which just goes though all (or some) of the IRQ resources and hooks
them up to interrupt handlers. The mangling of data between struct
resource and interrupt numbers or base address and size are extremely
common - passing some higher level data structure to do a batch
operation of ioremap() or request_irq() would be nice to have IMO.
> Things work reasonably well in PCI land since you have at least a fairly
> logical and consistent abstraction in terms of how the BARs are arranged,
> but even then the request/remap helpers take a BAR mask to work out which
> ones to deal with in what way (your ioremap()'ed window will want to be
> requested, but not all requested windows will want to be remapped). You
> could probably do this for platform devices too if you really wanted to,
> but you're not likely to be able to generalize much beyond that.
I'm mainly interested in platform devices here, so that may be
something to look in to.
> The end result however is that you're still required to deal with
> flagging the region (or the parts of the region your driver is
> immediately concerned with) as busy, so dropping it as you are doing here
> is unacceptable regardless of whether you are using a helper to make life
> easier or not.
I'm not arguing saying that the patch is correct. It's obviously not.
I just happen to dislike the "correct" code.
Please drop this patch from the series and use the recently posted V2
patch that makes use of request_mem_region.:
[PATCH] fbdev: sh_mipi_dsi: Require two I/O resources V2
Thanks,
/ magnus
On Wed, Nov 17, 2010 at 03:44:05PM +0900, Magnus Damm wrote:
> fbdev: SH-Mobile MIPI-DSI Update
>
> [PATCH 01/05] fbdev: sh_mipi_dsi: Remove request/release mem region
> [PATCH 02/05] fbdev: sh_mipi_dsi: Make use of register names
> [PATCH 03/05] ARM: mach-shmobile: Extend AP4EVB MIPI-DSI resources
> [PATCH 04/05] fbdev: sh_mipi_dsi: Require two I/O resources
> [PATCH 05/05] fbdev: sh_mipi_dsi: Allow LCDC board callbacks
>
Ok, I've applied the updated version of this with 01/05 dropped and 04/05
replaced with the V2 version. In the future make sure to Cc the
linux-fbdev list, too.