Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

[PATCH] V4L/DVB: mx1-camera: compile fix

1 view
Skip to first unread message

Uwe Kleine-König

unread,
Mar 4, 2010, 12:00:03 PM3/4/10
to
This is a regression of

7d58289 (mx1: prefix SOC specific defines with MX1_ and deprecate old names)

Signed-off-by: Uwe Kleine-König <u.klein...@pengutronix.de>
---
Hello,

this went unnoticed up to now as mx1_defconfig doesn't include support
for mx1-camera.
I have a patch pending to change that though.

Best regards
Uwe

drivers/media/video/mx1_camera.c | 5 ++++-
1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/mx1_camera.c b/drivers/media/video/mx1_camera.c
index 2ba14fb..38e5315 100644
--- a/drivers/media/video/mx1_camera.c
+++ b/drivers/media/video/mx1_camera.c
@@ -45,6 +45,9 @@
#include <mach/hardware.h>
#include <mach/mx1_camera.h>

+#undef DMA_BASE
+#define DMA_BASE MX1_IO_ADDRESS(MX1_DMA_BASE_ADDR)
+
/*
* CSI registers
*/
@@ -783,7 +786,7 @@ static int __init mx1_camera_probe(struct platform_device *pdev)
pcdev);

imx_dma_config_channel(pcdev->dma_chan, IMX_DMA_TYPE_FIFO,
- IMX_DMA_MEMSIZE_32, DMA_REQ_CSI_R, 0);
+ IMX_DMA_MEMSIZE_32, MX1_DMA_REQ_CSI_R, 0);
/* burst length : 16 words = 64 bytes */
imx_dma_config_burstlen(pcdev->dma_chan, 0);

--
1.7.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Guennadi Liakhovetski

unread,
Mar 4, 2010, 12:20:02 PM3/4/10
to
Hi Uwe

On Thu, 4 Mar 2010, Uwe Kleine-König wrote:

> This is a regression of
>
> 7d58289 (mx1: prefix SOC specific defines with MX1_ and deprecate old names)
>
> Signed-off-by: Uwe Kleine-König <u.klein...@pengutronix.de>
> ---
> Hello,
>
> this went unnoticed up to now as mx1_defconfig doesn't include support
> for mx1-camera.
> I have a patch pending to change that though.
>
> Best regards
> Uwe
>
> drivers/media/video/mx1_camera.c | 5 ++++-
> 1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/media/video/mx1_camera.c b/drivers/media/video/mx1_camera.c
> index 2ba14fb..38e5315 100644
> --- a/drivers/media/video/mx1_camera.c
> +++ b/drivers/media/video/mx1_camera.c
> @@ -45,6 +45,9 @@
> #include <mach/hardware.h>
> #include <mach/mx1_camera.h>
>
> +#undef DMA_BASE
> +#define DMA_BASE MX1_IO_ADDRESS(MX1_DMA_BASE_ADDR)

I don't like this. Why the "undef"? Is DMA_BASE already defined? where and
what is it? If it is - we better use a different name, if not - just
remove the undef, please.

> +
> /*
> * CSI registers
> */
> @@ -783,7 +786,7 @@ static int __init mx1_camera_probe(struct platform_device *pdev)
> pcdev);
>
> imx_dma_config_channel(pcdev->dma_chan, IMX_DMA_TYPE_FIFO,
> - IMX_DMA_MEMSIZE_32, DMA_REQ_CSI_R, 0);
> + IMX_DMA_MEMSIZE_32, MX1_DMA_REQ_CSI_R, 0);
> /* burst length : 16 words = 64 bytes */
> imx_dma_config_burstlen(pcdev->dma_chan, 0);
>
> --
> 1.7.0
>

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Uwe Kleine-König

unread,
Mar 4, 2010, 2:30:01 PM3/4/10
to
Hi Guennadi,

On Thu, Mar 04, 2010 at 06:13:38PM +0100, Guennadi Liakhovetski wrote:
> > +#undef DMA_BASE
> > +#define DMA_BASE MX1_IO_ADDRESS(MX1_DMA_BASE_ADDR)
>
> I don't like this. Why the "undef"? Is DMA_BASE already defined? where and
> what is it? If it is - we better use a different name, if not - just
> remove the undef, please.

yes, it's not pretty, but I wanted to make a minimal patch.

arch/arm/plat-mxc/include/mach/dma-mx1-mx2.h has:

#define DMA_BASE IO_ADDRESS(DMA_BASE_ADDR)

so that was used before. I don't really know the driver, just made it
compile again. If you have a nice suggestion, I will happily implement
it.

Best regards
Uwe

--
Pengutronix e.K. | Uwe Kleine-K�nig |
Industrial Linux Solutions | http://www.pengutronix.de/ |

Sascha Hauer

unread,
Mar 4, 2010, 2:50:03 PM3/4/10
to
On Thu, Mar 04, 2010 at 08:26:23PM +0100, Uwe Kleine-Kï¿œnig wrote:
> Hi Guennadi,
>
> On Thu, Mar 04, 2010 at 06:13:38PM +0100, Guennadi Liakhovetski wrote:
> > > +#undef DMA_BASE
> > > +#define DMA_BASE MX1_IO_ADDRESS(MX1_DMA_BASE_ADDR)
> >
> > I don't like this. Why the "undef"? Is DMA_BASE already defined? where and
> > what is it? If it is - we better use a different name, if not - just
> > remove the undef, please.
> yes, it's not pretty, but I wanted to make a minimal patch.
>
> arch/arm/plat-mxc/include/mach/dma-mx1-mx2.h has:
>
> #define DMA_BASE IO_ADDRESS(DMA_BASE_ADDR)

This is only used in the mx1 camera driver, so you can just remove it
from dma-mx1-mx2.h and use MX1_IO_ADDRESS(MX1_DMA_BASE_ADDR) in the camera
driver.

Sascha

--
Pengutronix e.K. | |


Industrial Linux Solutions | http://www.pengutronix.de/ |

Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

Uwe Kleine-König

unread,
Mar 5, 2010, 5:50:01 AM3/5/10
to
This is a regression of

7d58289 (mx1: prefix SOC specific defines with MX1_ and deprecate old names)

Signed-off-by: Uwe Kleine-König <u.klein...@pengutronix.de>
---

drivers/media/video/mx1_camera.c | 12 +++++++-----
1 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/media/video/mx1_camera.c b/drivers/media/video/mx1_camera.c
index 2ba14fb..29c2833 100644
--- a/drivers/media/video/mx1_camera.c
+++ b/drivers/media/video/mx1_camera.c
@@ -45,11 +45,13 @@
#include <mach/hardware.h>
#include <mach/mx1_camera.h>

+#define __DMAREG(offset) (MX1_IO_ADDRESS(MX1_DMA_BASE_ADDR) + offset)


+
/*
* CSI registers
*/

-#define DMA_CCR(x) (0x8c + ((x) << 6)) /* Control Registers */
-#define DMA_DIMR 0x08 /* Interrupt mask Register */
+#define DMA_CCR(x) __DMAREG(0x8c + ((x) << 6)) /* Control Registers */
+#define DMA_DIMR __DMAREG(0x08) /* Interrupt mask Register */
#define CSICR1 0x00 /* CSI Control Register 1 */
#define CSISR 0x08 /* CSI Status Register */
#define CSIRXR 0x10 /* CSI RxFIFO Register */
@@ -783,7 +785,7 @@ static int __init mx1_camera_probe(struct platform_device *pdev)


pcdev);

imx_dma_config_channel(pcdev->dma_chan, IMX_DMA_TYPE_FIFO,
- IMX_DMA_MEMSIZE_32, DMA_REQ_CSI_R, 0);
+ IMX_DMA_MEMSIZE_32, MX1_DMA_REQ_CSI_R, 0);
/* burst length : 16 words = 64 bytes */
imx_dma_config_burstlen(pcdev->dma_chan, 0);

@@ -797,8 +799,8 @@ static int __init mx1_camera_probe(struct platform_device *pdev)
set_fiq_handler(&mx1_camera_sof_fiq_start, &mx1_camera_sof_fiq_end -
&mx1_camera_sof_fiq_start);

- regs.ARM_r8 = DMA_BASE + DMA_DIMR;
- regs.ARM_r9 = DMA_BASE + DMA_CCR(pcdev->dma_chan);
+ regs.ARM_r8 = (long)DMA_DIMR;
+ regs.ARM_r9 = (long)DMA_CCR(pcdev->dma_chan);
regs.ARM_r10 = (long)pcdev->base + CSICR1;
regs.ARM_fp = (long)pcdev->base + CSISR;
regs.ARM_sp = 1 << pcdev->dma_chan;
--
1.7.0

Guennadi Liakhovetski

unread,
Mar 12, 2010, 4:30:03 AM3/12/10
to
Hi Uwe

On Fri, 5 Mar 2010, Uwe Kleine-König wrote:

> This is a regression of
>
> 7d58289 (mx1: prefix SOC specific defines with MX1_ and deprecate old names)
>
> Signed-off-by: Uwe Kleine-König <u.klein...@pengutronix.de>
> ---
> drivers/media/video/mx1_camera.c | 12 +++++++-----
> 1 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/media/video/mx1_camera.c b/drivers/media/video/mx1_camera.c
> index 2ba14fb..29c2833 100644
> --- a/drivers/media/video/mx1_camera.c
> +++ b/drivers/media/video/mx1_camera.c
> @@ -45,11 +45,13 @@
> #include <mach/hardware.h>
> #include <mach/mx1_camera.h>
>
> +#define __DMAREG(offset) (MX1_IO_ADDRESS(MX1_DMA_BASE_ADDR) + offset)
> +

Well, I think, Sascha is right, we have to fix
arch/arm/plat-mxc/include/mach/dma-mx1-mx2.h, because that's what actually
got broken. The line

#define DMA_BASE IO_ADDRESS(DMA_BASE_ADDR)

in it is no longer valid, right? So, we have to either remove it, or fix
it, if we think, that other drivers might start using it. And even if we
decide to remove it from the header and implement here, wouldn't it be
better to choose a name, not beginning with "__"? Something like
MX1_DMA_REG, perhaps? Or maybe even we shall remap those registers?

Thanks
Guennadi

---


Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Guennadi Liakhovetski

unread,
Mar 12, 2010, 5:00:02 AM3/12/10
to
On Fri, 12 Mar 2010, Uwe Kleine-König wrote:

> > Or maybe even we shall remap those registers?

> Well, they are remapped, don't they? Otherwise IO_ADDRESS wouldn't
> work.

Yes, they are (statically, I presume), but not in _this_ driver...

Thanks
Guennadi

Uwe Kleine-König

unread,
Mar 12, 2010, 5:00:03 AM3/12/10
to
Hello,

On Fri, Mar 12, 2010 at 10:22:31AM +0100, Guennadi Liakhovetski wrote:


> On Fri, 5 Mar 2010, Uwe Kleine-K�nig wrote:
> > This is a regression of
> >
> > 7d58289 (mx1: prefix SOC specific defines with MX1_ and deprecate old names)
> >

> > Signed-off-by: Uwe Kleine-K�nig <u.klein...@pengutronix.de>


> > ---
> > drivers/media/video/mx1_camera.c | 12 +++++++-----
> > 1 files changed, 7 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/media/video/mx1_camera.c b/drivers/media/video/mx1_camera.c
> > index 2ba14fb..29c2833 100644
> > --- a/drivers/media/video/mx1_camera.c
> > +++ b/drivers/media/video/mx1_camera.c
> > @@ -45,11 +45,13 @@
> > #include <mach/hardware.h>
> > #include <mach/mx1_camera.h>
> >
> > +#define __DMAREG(offset) (MX1_IO_ADDRESS(MX1_DMA_BASE_ADDR) + offset)
> > +
>
> Well, I think, Sascha is right, we have to fix
> arch/arm/plat-mxc/include/mach/dma-mx1-mx2.h, because that's what actually
> got broken. The line
>
> #define DMA_BASE IO_ADDRESS(DMA_BASE_ADDR)
>
> in it is no longer valid, right? So, we have to either remove it, or fix
> it, if we think, that other drivers might start using it.

I thought a minimal fix would be a good idea. I have no problem with a
clean one though.

> And even if we
> decide to remove it from the header and implement here, wouldn't it be
> better to choose a name, not beginning with "__"? Something like
> MX1_DMA_REG, perhaps?

Then the register definitions should go into the header, too. I will
prepare a patch later today.


> Or maybe even we shall remap those registers?

Well, they are remapped, don't they? Otherwise IO_ADDRESS wouldn't
work.

Best regards


Uwe
--
Pengutronix e.K. | Uwe Kleine-K�nig |

Industrial Linux Solutions | http://www.pengutronix.de/ |

Uwe Kleine-König

unread,
Mar 16, 2010, 6:00:02 AM3/16/10
to
This is a regression of

7d58289 (mx1: prefix SOC specific defines with MX1_ and deprecate old names)

Signed-off-by: Uwe Kleine-König <u.klein...@pengutronix.de>
---
arch/arm/plat-mxc/include/mach/dma-mx1-mx2.h | 4 +++-
drivers/media/video/mx1_camera.c | 8 +++-----
2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/arch/arm/plat-mxc/include/mach/dma-mx1-mx2.h b/arch/arm/plat-mxc/include/mach/dma-mx1-mx2.h
index 07be8ad..d25a65f 100644
--- a/arch/arm/plat-mxc/include/mach/dma-mx1-mx2.h
+++ b/arch/arm/plat-mxc/include/mach/dma-mx1-mx2.h
@@ -31,7 +31,9 @@
#define DMA_MODE_WRITE 1
#define DMA_MODE_MASK 1

-#define DMA_BASE IO_ADDRESS(DMA_BASE_ADDR)
+#define MX1_DMA_REG(offset) MX1_IO_ADDRESS(MX1_DMA_BASE_ADDR + offset)
+#define MX1_DMA_CCR(x) MX1_DMA_REG(0x8c + ((x) << 6))
+#define MX1_DMA_DIMR MX1_DMA_REG(0x08)

#define IMX_DMA_MEMSIZE_32 (0 << 4)
#define IMX_DMA_MEMSIZE_8 (1 << 4)
diff --git a/drivers/media/video/mx1_camera.c b/drivers/media/video/mx1_camera.c
index c167cc3..aa81acd 100644
--- a/drivers/media/video/mx1_camera.c
+++ b/drivers/media/video/mx1_camera.c
@@ -48,8 +48,6 @@


/*
* CSI registers
*/
-#define DMA_CCR(x) (0x8c + ((x) << 6)) /* Control Registers */
-#define DMA_DIMR 0x08 /* Interrupt mask Register */

#define CSICR1 0x00 /* CSI Control Register 1 */
#define CSISR 0x08 /* CSI Status Register */
#define CSIRXR 0x10 /* CSI RxFIFO Register */

@@ -783,7 +781,7 @@ static int __init mx1_camera_probe(struct platform_device *pdev)


pcdev);

imx_dma_config_channel(pcdev->dma_chan, IMX_DMA_TYPE_FIFO,
- IMX_DMA_MEMSIZE_32, DMA_REQ_CSI_R, 0);
+ IMX_DMA_MEMSIZE_32, MX1_DMA_REQ_CSI_R, 0);
/* burst length : 16 words = 64 bytes */
imx_dma_config_burstlen(pcdev->dma_chan, 0);

@@ -797,8 +795,8 @@ static int __init mx1_camera_probe(struct platform_device *pdev)


set_fiq_handler(&mx1_camera_sof_fiq_start, &mx1_camera_sof_fiq_end -
&mx1_camera_sof_fiq_start);

- regs.ARM_r8 = DMA_BASE + DMA_DIMR;
- regs.ARM_r9 = DMA_BASE + DMA_CCR(pcdev->dma_chan);

+ regs.ARM_r8 = (long)MX1_DMA_DIMR;
+ regs.ARM_r9 = (long)MX1_DMA_CCR(pcdev->dma_chan);


regs.ARM_r10 = (long)pcdev->base + CSICR1;
regs.ARM_fp = (long)pcdev->base + CSISR;
regs.ARM_sp = 1 << pcdev->dma_chan;
--
1.7.0

--

Guennadi Liakhovetski

unread,
Mar 16, 2010, 7:10:01 AM3/16/10
to
On Tue, 16 Mar 2010, Uwe Kleine-König wrote:

> This is a regression of
>
> 7d58289 (mx1: prefix SOC specific defines with MX1_ and deprecate old names)
>

> Signed-off-by: Uwe Kleine-König <u.klein...@pengutronix.de>


> ---
> arch/arm/plat-mxc/include/mach/dma-mx1-mx2.h | 4 +++-
> drivers/media/video/mx1_camera.c | 8 +++-----
> 2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/arch/arm/plat-mxc/include/mach/dma-mx1-mx2.h b/arch/arm/plat-mxc/include/mach/dma-mx1-mx2.h
> index 07be8ad..d25a65f 100644
> --- a/arch/arm/plat-mxc/include/mach/dma-mx1-mx2.h
> +++ b/arch/arm/plat-mxc/include/mach/dma-mx1-mx2.h
> @@ -31,7 +31,9 @@
> #define DMA_MODE_WRITE 1
> #define DMA_MODE_MASK 1
>
> -#define DMA_BASE IO_ADDRESS(DMA_BASE_ADDR)
> +#define MX1_DMA_REG(offset) MX1_IO_ADDRESS(MX1_DMA_BASE_ADDR + offset)

Here offset must be in parenthesis - plus is not the lowest-prio
operation, so, someone might well call MX1_DMA_REG(x << 1), which is also
almost what you do below, and this is a header...

> +#define MX1_DMA_CCR(x) MX1_DMA_REG(0x8c + ((x) << 6))
> +#define MX1_DMA_DIMR MX1_DMA_REG(0x08)

Why don't we also take comments to these registers - I promise, I won't
complain about > 80 characters;)

Yes, I think, that looks good now. Just correct the above issues and I'll
ack it.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Uwe Kleine-König

unread,
Mar 27, 2010, 5:50:01 PM3/27/10
to
This fixes a regression of

7d58289 (mx1: prefix SOC specific defines with MX1_ and deprecate old names)

Signed-off-by: Uwe Kleine-König <u.klein...@pengutronix.de>
---
arch/arm/plat-mxc/include/mach/dma-mx1-mx2.h | 8 +++++++-
drivers/media/video/mx1_camera.c | 8 +++-----
2 files changed, 10 insertions(+), 6 deletions(-)

Hello,

changed since last post:

- put offset in the definition of MX1_DMA_REG in parenthesis
- describe register definitions now moved to dma-mx1-mx2.h with the full
register names and order by address.

Thanks
Uwe

diff --git a/arch/arm/plat-mxc/include/mach/dma-mx1-mx2.h b/arch/arm/plat-mxc/include/mach/dma-mx1-mx2.h
index 07be8ad..4b63b05 100644
--- a/arch/arm/plat-mxc/include/mach/dma-mx1-mx2.h
+++ b/arch/arm/plat-mxc/include/mach/dma-mx1-mx2.h
@@ -31,7 +31,13 @@


#define DMA_MODE_WRITE 1
#define DMA_MODE_MASK 1

-#define DMA_BASE IO_ADDRESS(DMA_BASE_ADDR)

+#define MX1_DMA_REG(offset) MX1_IO_ADDRESS(MX1_DMA_BASE_ADDR + (offset))
+
+/* DMA Interrupt Mask Register */
+#define MX1_DMA_DIMR MX1_DMA_REG(0x08)
+
+/* Channel Control Register */


+#define MX1_DMA_CCR(x) MX1_DMA_REG(0x8c + ((x) << 6))

--
1.7.0

Guennadi Liakhovetski

unread,
Mar 28, 2010, 4:20:02 PM3/28/10
to
On Sat, 27 Mar 2010, Uwe Kleine-König wrote:

> This fixes a regression of
>
> 7d58289 (mx1: prefix SOC specific defines with MX1_ and deprecate old names)
>
> Signed-off-by: Uwe Kleine-König <u.klein...@pengutronix.de>

Sascha, I need your ack to pull this via my tree.

Thanks
Guennadi

---


Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Sascha Hauer

unread,
Mar 31, 2010, 4:50:01 AM3/31/10
to
On Sun, Mar 28, 2010 at 10:12:06PM +0200, Guennadi Liakhovetski wrote:
> On Sat, 27 Mar 2010, Uwe Kleine-König wrote:
>
> > This fixes a regression of
> >
> > 7d58289 (mx1: prefix SOC specific defines with MX1_ and deprecate old names)
> >
> > Signed-off-by: Uwe Kleine-König <u.klein...@pengutronix.de>
>
> Sascha, I need your ack to pull this via my tree.

Here we go:

Acked-by: Sascha Hauer <s.h...@pengutronix.de>

Sascha


--
Pengutronix e.K. | |


Industrial Linux Solutions | http://www.pengutronix.de/ |

Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

0 new messages