Re: [PATCH 8/8] marvell-cam: Basic working MMP camera driver
On Thu, 16 Jun 2011 10:37:37 +0800
Kassey Lee kassey1...@gmail.com wrote:
+static void mmpcam_power_down(struct mcam_camera *mcam)
+{
+ struct mmp_camera *cam = mcam_to_cam(mcam);
+ struct mmp_camera_platform_data *pdata;
+/*
+ * Turn off clocks and set reset lines
+ */
+ iowrite32(0, cam-power_regs + REG_CCIC_DCGCR);
+ iowrite32(0, cam-power_regs + REG_CCIC_CRCR);
+/*
+ * Shut down the sensor.
+ */
+ pdata = cam-pdev-dev.platform_data;
+ gpio_set_value(pdata-sensor_power_gpio, 0);
+ gpio_set_value(pdata-sensor_reset_gpio, 0);
it is better to have a callback function to controller sensor power on/off.
and place the callback function in board.c
This is an interesting question, actually. The problem is that board
files are on their way out; it's going to be very hard to get any more
board files into the mainline going forward.
The mmp-camera driver does depend on a board file, but I've been careful
to restrict things to basic platform data which can just as easily be put
into a device tree. Power management callbacks don't really qualify.
So it seems that we need to figure out a way to push this kind of
pin/power management down into the sensor-specific code. Looking at the
subdev stuff, it looks like a bit of thought has been put into that
direction; there's the s_io_pin_config() callback to describe pins to the
sensor. But it's almost entirely unused.
There is no power up/down callback, currently. We could ponder on
whether one should be added, or whether this should be handled through the
existing power management code somehow. I honestly don't know what the
best answer is on this one - will have to do some digging. Suggestions
welcome.
Thanks,
jon
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] marvell-cam: Basic working MMP camera driver
Hello,
On 06/16/2011 05:17 PM, Jonathan Corbet wrote:
On Thu, 16 Jun 2011 10:37:37 +0800
Kassey Leekassey1...@gmail.com wrote:
+static void mmpcam_power_down(struct mcam_camera *mcam)
+{
+ struct mmp_camera *cam = mcam_to_cam(mcam);
+ struct mmp_camera_platform_data *pdata;
+/*
+ * Turn off clocks and set reset lines
+ */
+ iowrite32(0, cam-power_regs + REG_CCIC_DCGCR);
+ iowrite32(0, cam-power_regs + REG_CCIC_CRCR);
+/*
+ * Shut down the sensor.
+ */
+ pdata = cam-pdev-dev.platform_data;
+ gpio_set_value(pdata-sensor_power_gpio, 0);
+ gpio_set_value(pdata-sensor_reset_gpio, 0);
it is better to have a callback function to controller sensor power on/off.
and place the callback function in board.c
It might be more convenient for the driver writer but I do not think
it is generally better. Platform callbacks cannot really be migrated
to FDT. For reasons pointed out by Jon below I have also been trying
to avoid callbacks in sensors' platform data.
This is an interesting question, actually. The problem is that board
files are on their way out; it's going to be very hard to get any more
board files into the mainline going forward.
The mmp-camera driver does depend on a board file, but I've been careful
to restrict things to basic platform data which can just as easily be put
into a device tree. Power management callbacks don't really qualify.
So it seems that we need to figure out a way to push this kind of
pin/power management down into the sensor-specific code. Looking at the
subdev stuff, it looks like a bit of thought has been put into that
direction; there's the s_io_pin_config() callback to describe pins to the
But having sensor's GPIOs configured by bridge driver does not help us
much here, does it? The bridge driver would still need to retrieve the
sensor configuration from somewhere, since it is machine/board specific.
And it's the subdev driver that will know how the GPIOs should be handled,
the timing constraints, etc. So as long as we can pass gpio numbers from
a device tree to subdevs we probably do not need an additional subdev
operation.
I guess when there come more subdev drivers dealing with GPIOs we could try
and see what could be generalized.
sensor. But it's almost entirely unused.
There is no power up/down callback, currently. We could ponder on
We have the subdev s_power core operation in struct v4l2_subdev_core_ops.
However it seems that its semantic is not yet clearly documented.
whether one should be added, or whether this should be handled through the
existing power management code somehow. I honestly don't know what the
best answer is on this one - will have to do some digging. Suggestions
welcome.
--
Regards,
Sylwester
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] marvell-cam: Basic working MMP camera driver
Em 16-06-2011 16:44, Sylwester Nawrocki escreveu:
Hello,
On 06/16/2011 05:17 PM, Jonathan Corbet wrote:
On Thu, 16 Jun 2011 10:37:37 +0800
Kassey Leekassey1...@gmail.com wrote:
+static void mmpcam_power_down(struct mcam_camera *mcam)
+{
+ struct mmp_camera *cam = mcam_to_cam(mcam);
+ struct mmp_camera_platform_data *pdata;
+/*
+ * Turn off clocks and set reset lines
+ */
+ iowrite32(0, cam-power_regs + REG_CCIC_DCGCR);
+ iowrite32(0, cam-power_regs + REG_CCIC_CRCR);
+/*
+ * Shut down the sensor.
+ */
+ pdata = cam-pdev-dev.platform_data;
+ gpio_set_value(pdata-sensor_power_gpio, 0);
+ gpio_set_value(pdata-sensor_reset_gpio, 0);
it is better to have a callback function to controller sensor power on/off.
and place the callback function in board.c
It might be more convenient for the driver writer but I do not think
it is generally better. Platform callbacks cannot really be migrated
to FDT. For reasons pointed out by Jon below I have also been trying
to avoid callbacks in sensors' platform data.
Well, outside the SoC world, we use two different approaches for GPIO:
The first and more used one is to add the GPIO settings into a per-card model
table (see the tables at cx88-cards, em28xx-cards, etc). Once the driver
detects what's the card model (by USB ID or PCI ID), it selects one
line at the board table entry. This works better than callbacks, and are
very simple to code.
Unfortunately, on a few cases, the GPIO's need to be set during a subdev
operation, on several tuners, like the Xceive family of tuners, that require
a chip reset via GPIO during firmware load.
This is an interesting question, actually. The problem is that board
files are on their way out; it's going to be very hard to get any more
board files into the mainline going forward.
The mmp-camera driver does depend on a board file, but I've been careful
to restrict things to basic platform data which can just as easily be put
into a device tree. Power management callbacks don't really qualify.
So it seems that we need to figure out a way to push this kind of
pin/power management down into the sensor-specific code. Looking at the
subdev stuff, it looks like a bit of thought has been put into that
direction; there's the s_io_pin_config() callback to describe pins to the
But having sensor's GPIOs configured by bridge driver does not help us
much here, does it? The bridge driver would still need to retrieve the
sensor configuration from somewhere, since it is machine/board specific.
And it's the subdev driver that will know how the GPIOs should be handled,
the timing constraints, etc. So as long as we can pass gpio numbers from
a device tree to subdevs we probably do not need an additional subdev
operation.
I guess when there come more subdev drivers dealing with GPIOs we could try
and see what could be generalized.
On my experiences, the timings and even the meanings for GPIO's are not subdev
specific, at least outside SoC world. Especially when you have multiple devices
sharing the same I2C bus, timings need to be adjusted to avoid troubles with
other chips, and due to capacitance effects at the board tracks.
sensor. But it's almost entirely unused.
There is no power up/down callback, currently. We could ponder on
We have the subdev s_power core operation in struct v4l2_subdev_core_ops.
However it seems that its semantic is not yet clearly documented.
Yes, s_power is meant to be used for putting a sub-device to sleep or for
waking it up.
whether one should be added, or whether this should be handled through the
existing power management code somehow. I honestly don't know what the
best answer is on this one - will have to do some digging. Suggestions
welcome.
--
Regards,
Sylwester
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 8/8] marvell-cam: Basic working MMP camera driver
2011/6/12 Jonathan Corbet cor...@lwn.net:
Now we have a camera working over the marvell cam controller core. It
works like the cafe driver and has all the same limitations, contiguous DMA
only being one of them. But it's a start.
Signed-off-by: Jonathan Corbet cor...@lwn.net
---
drivers/media/video/Makefile | 1 +
drivers/media/video/marvell-ccic/Kconfig | 11 +
drivers/media/video/marvell-ccic/Makefile | 4 +
drivers/media/video/marvell-ccic/mcam-core.c | 28 ++-
drivers/media/video/marvell-ccic/mmp-driver.c | 339
+
include/media/mmp-camera.h | 9 +
include/media/v4l2-chip-ident.h | 3 +-
7 files changed, 386 insertions(+), 9 deletions(-)
create mode 100644 drivers/media/video/marvell-ccic/mmp-driver.c
create mode 100644 include/media/mmp-camera.h
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index 42b6a7a..89478f0 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -128,6 +128,7 @@ obj-$(CONFIG_VIDEO_M32R_AR_M64278) += arv.o
obj-$(CONFIG_VIDEO_CX2341X) += cx2341x.o
obj-$(CONFIG_VIDEO_CAFE_CCIC) += marvell-ccic/
+obj-$(CONFIG_VIDEO_MMP_CAMERA) += marvell-ccic/
obj-$(CONFIG_VIDEO_VIA_CAMERA) += via-camera.o
diff --git a/drivers/media/video/marvell-ccic/Kconfig
b/drivers/media/video/marvell-ccic/Kconfig
index 80136a8..b4f7260 100644
--- a/drivers/media/video/marvell-ccic/Kconfig
+++ b/drivers/media/video/marvell-ccic/Kconfig
@@ -7,3 +7,14 @@ config VIDEO_CAFE_CCIC
CMOS camera controller. This is the controller found on first-
generation OLPC systems.
+config VIDEO_MMP_CAMERA
+ tristate Marvell Armada 610 integrated camera controller support
+ depends on ARCH_MMP I2C VIDEO_V4L2
+ select VIDEO_OV7670
+ select I2C_GPIO
+ ---help---
+ This is a Video4Linux2 driver for the integrated camera
+ controller found on Marvell Armada 610 application
+ processors (and likely beyond). This is the controller found
+ in OLPC XO 1.75 systems.
+
diff --git a/drivers/media/video/marvell-ccic/Makefile
b/drivers/media/video/marvell-ccic/Makefile
index 462b385c..05a792c 100644
--- a/drivers/media/video/marvell-ccic/Makefile
+++ b/drivers/media/video/marvell-ccic/Makefile
@@ -1,2 +1,6 @@
obj-$(CONFIG_VIDEO_CAFE_CCIC) += cafe_ccic.o
cafe_ccic-y := cafe-driver.o mcam-core.o
+
+obj-$(CONFIG_VIDEO_MMP_CAMERA) += mmp_camera.o
+mmp_camera-y := mmp-driver.o mcam-core.o
+
diff --git a/drivers/media/video/marvell-ccic/mcam-core.c
b/drivers/media/video/marvell-ccic/mcam-core.c
index 014b70b..3e6a5e8 100644
--- a/drivers/media/video/marvell-ccic/mcam-core.c
+++ b/drivers/media/video/marvell-ccic/mcam-core.c
@@ -167,7 +167,7 @@ static void mcam_set_config_needed(struct mcam_camera
*cam, int needed)
/*
- * Debugging and related. FIXME these are broken
+ * Debugging and related.
*/
#define cam_err(cam, fmt, arg...) \
dev_err((cam)-dev, fmt, ##arg);
@@ -202,7 +202,8 @@ static void mcam_ctlr_dma(struct mcam_camera *cam)
mcam_reg_clear_bit(cam, REG_CTRL1, C1_TWOBUFS);
} else
mcam_reg_set_bit(cam, REG_CTRL1, C1_TWOBUFS);
- mcam_reg_write(cam, REG_UBAR, 0); /* 32 bits only for now */
+ if (cam-chip_id == V4L2_IDENT_CAFE)
+ mcam_reg_write(cam, REG_UBAR, 0); /* 32 bits only */
}
static void mcam_ctlr_image(struct mcam_camera *cam)
@@ -358,8 +359,8 @@ static void mcam_ctlr_power_up(struct mcam_camera *cam)
unsigned long flags;
spin_lock_irqsave(cam-dev_lock, flags);
- mcam_reg_clear_bit(cam, REG_CTRL1, C1_PWRDWN);
cam-plat_power_up(cam);
+ mcam_reg_clear_bit(cam, REG_CTRL1, C1_PWRDWN);
spin_unlock_irqrestore(cam-dev_lock, flags);
msleep(5); /* Just to be sure */
}
@@ -369,8 +370,13 @@ static void mcam_ctlr_power_down(struct mcam_camera *cam)
unsigned long flags;
spin_lock_irqsave(cam-dev_lock, flags);
- cam-plat_power_down(cam);
+ /*
+ * School of hard knocks department: be sure we do any register
+ * twiddling on the controller *before* calling the platform
+ * power down routine.
+ */
mcam_reg_set_bit(cam, REG_CTRL1, C1_PWRDWN);
+ cam-plat_power_down(cam);
spin_unlock_irqrestore(cam-dev_lock, flags);
}
@@ -1622,14 +1628,20 @@ out_unregister:
void mccic_shutdown(struct mcam_camera *cam)
{
- if (cam-users 0)
+ /*
+ * If we have no users (and we really, really should have no
+ * users) the device will already be powered down. Trying to
+ * take it down again will wedge the machine, which is frowned
+ * upon.
+ */
+ if (cam-users 0) {
cam_warn(cam, Removing a device with users!\n);
+
[PATCH 8/8] marvell-cam: Basic working MMP camera driver
Now we have a camera working over the marvell cam controller core. It
works like the cafe driver and has all the same limitations, contiguous DMA
only being one of them. But it's a start.
Signed-off-by: Jonathan Corbet cor...@lwn.net
---
drivers/media/video/Makefile |1 +
drivers/media/video/marvell-ccic/Kconfig | 11 +
drivers/media/video/marvell-ccic/Makefile |4 +
drivers/media/video/marvell-ccic/mcam-core.c | 28 ++-
drivers/media/video/marvell-ccic/mmp-driver.c | 339 +
include/media/mmp-camera.h|9 +
include/media/v4l2-chip-ident.h |3 +-
7 files changed, 386 insertions(+), 9 deletions(-)
create mode 100644 drivers/media/video/marvell-ccic/mmp-driver.c
create mode 100644 include/media/mmp-camera.h
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index 42b6a7a..89478f0 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -128,6 +128,7 @@ obj-$(CONFIG_VIDEO_M32R_AR_M64278) += arv.o
obj-$(CONFIG_VIDEO_CX2341X) += cx2341x.o
obj-$(CONFIG_VIDEO_CAFE_CCIC) += marvell-ccic/
+obj-$(CONFIG_VIDEO_MMP_CAMERA) += marvell-ccic/
obj-$(CONFIG_VIDEO_VIA_CAMERA) += via-camera.o
diff --git a/drivers/media/video/marvell-ccic/Kconfig
b/drivers/media/video/marvell-ccic/Kconfig
index 80136a8..b4f7260 100644
--- a/drivers/media/video/marvell-ccic/Kconfig
+++ b/drivers/media/video/marvell-ccic/Kconfig
@@ -7,3 +7,14 @@ config VIDEO_CAFE_CCIC
CMOS camera controller. This is the controller found on first-
generation OLPC systems.
+config VIDEO_MMP_CAMERA
+ tristate Marvell Armada 610 integrated camera controller support
+ depends on ARCH_MMP I2C VIDEO_V4L2
+ select VIDEO_OV7670
+ select I2C_GPIO
+ ---help---
+ This is a Video4Linux2 driver for the integrated camera
+ controller found on Marvell Armada 610 application
+ processors (and likely beyond). This is the controller found
+ in OLPC XO 1.75 systems.
+
diff --git a/drivers/media/video/marvell-ccic/Makefile
b/drivers/media/video/marvell-ccic/Makefile
index 462b385c..05a792c 100644
--- a/drivers/media/video/marvell-ccic/Makefile
+++ b/drivers/media/video/marvell-ccic/Makefile
@@ -1,2 +1,6 @@
obj-$(CONFIG_VIDEO_CAFE_CCIC) += cafe_ccic.o
cafe_ccic-y := cafe-driver.o mcam-core.o
+
+obj-$(CONFIG_VIDEO_MMP_CAMERA) += mmp_camera.o
+mmp_camera-y := mmp-driver.o mcam-core.o
+
diff --git a/drivers/media/video/marvell-ccic/mcam-core.c
b/drivers/media/video/marvell-ccic/mcam-core.c
index 014b70b..3e6a5e8 100644
--- a/drivers/media/video/marvell-ccic/mcam-core.c
+++ b/drivers/media/video/marvell-ccic/mcam-core.c
@@ -167,7 +167,7 @@ static void mcam_set_config_needed(struct mcam_camera *cam,
int needed)
/*
- * Debugging and related. FIXME these are broken
+ * Debugging and related.
*/
#define cam_err(cam, fmt, arg...) \
dev_err((cam)-dev, fmt, ##arg);
@@ -202,7 +202,8 @@ static void mcam_ctlr_dma(struct mcam_camera *cam)
mcam_reg_clear_bit(cam, REG_CTRL1, C1_TWOBUFS);
} else
mcam_reg_set_bit(cam, REG_CTRL1, C1_TWOBUFS);
- mcam_reg_write(cam, REG_UBAR, 0); /* 32 bits only for now */
+ if (cam-chip_id == V4L2_IDENT_CAFE)
+ mcam_reg_write(cam, REG_UBAR, 0); /* 32 bits only */
}
static void mcam_ctlr_image(struct mcam_camera *cam)
@@ -358,8 +359,8 @@ static void mcam_ctlr_power_up(struct mcam_camera *cam)
unsigned long flags;
spin_lock_irqsave(cam-dev_lock, flags);
- mcam_reg_clear_bit(cam, REG_CTRL1, C1_PWRDWN);
cam-plat_power_up(cam);
+ mcam_reg_clear_bit(cam, REG_CTRL1, C1_PWRDWN);
spin_unlock_irqrestore(cam-dev_lock, flags);
msleep(5); /* Just to be sure */
}
@@ -369,8 +370,13 @@ static void mcam_ctlr_power_down(struct mcam_camera *cam)
unsigned long flags;
spin_lock_irqsave(cam-dev_lock, flags);
- cam-plat_power_down(cam);
+ /*
+* School of hard knocks department: be sure we do any register
+* twiddling on the controller *before* calling the platform
+* power down routine.
+*/
mcam_reg_set_bit(cam, REG_CTRL1, C1_PWRDWN);
+ cam-plat_power_down(cam);
spin_unlock_irqrestore(cam-dev_lock, flags);
}
@@ -1622,14 +1628,20 @@ out_unregister:
void mccic_shutdown(struct mcam_camera *cam)
{
- if (cam-users 0)
+ /*
+* If we have no users (and we really, really should have no
+* users) the device will already be powered down. Trying to
+* take it down again will wedge the machine, which is frowned
+* upon.
+*/
+ if (cam-users 0) {
cam_warn(cam, Removing a device with users!\n);
+ mcam_ctlr_power_down(cam);
+ }
+ mcam_free_dma_bufs(cam);
if (cam-n_sbufs 0)
/* What if they are still
