RE: [PATCH v2] [media] marvell-ccic: simplify and fix clk handling (a bit)
Hi Russell,
>-Original Message-
>From: Russell King - ARM Linux [mailto:li...@arm.linux.org.uk]
>Sent: Thursday, September 26, 2013 4:24 PM
>To: Uwe Kleine-König
>Cc: Libin Yang; Jonathan Corbet; Mauro Carvalho Chehab;
>linux-media@vger.kernel.org;
>linux-arm-ker...@lists.infradead.org; ker...@pengutronix.de
>Subject: Re: [PATCH v2] [media] marvell-ccic: simplify and fix clk handling (a
>bit)
>
>On Thu, Sep 26, 2013 at 10:13:56AM +0200, Uwe Kleine-König wrote:
>> Hi Libin,
>>
>> On Wed, Sep 25, 2013 at 07:47:19PM -0700, Libin Yang wrote:
>> > In the clk enable and prepare function, we will check the NULL
>> > pointer. So it should be no problem.
>> I'm not sure what you mean here and unfortunately your quoting style
>> makes your statement appear without context.
>>
>> if (... && !IS_ERR(cam->mipi_clk)) {
>> if (cam->mipi_clk)
>> devm_clk_put(mcam->dev, cam->mipi_clk);
>> cam->mipi_clk = NULL;
>> }
>>
>> might work in your setup, but it's wrong usage of the clk API. There
>> is no reason NULL couldn't be a valid clk pointer. Moreover I cannot
>> find a place in that driver that calls prepare and/or enable for the
>> mipi_clk.
>> (BTW, calling clk_get_rate on a disabled clk is another thing you
>> shouldn't do.)
>
>It's a bug for another reason. Consider this:
>
> clk = devm_clk_get(...);
>
>Now, as the CLK API defines only IS_ERR(clk) to be errors, if clk is NULL then
>the devm
>API will allocate a tracking structure for the "allocated"
>clock. If you then do:
>
> if (!IS_ERR(clk)) {
> if (clk)
> devm_clk_put(clk);
> clk = NULL;
> }
>
>Then this structure won't get freed. Next time you call devm_clk_get(),
>you'll allocate another
>tracking structure. If the driver does this a lot, it will spawn lots of
>these tracking structures
>which will only get cleaned up when the device is unbound (possibly never.)
>
>So, what this driver is doing with its NULL checks against clocks is buggy, no
>two ways
>about it.
[Libin] Yes, you are right. it will not release the clk tracking structure if
it is NULL and may allocate again later. It is a bug.
Regards,
Libin
--
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 v2] [media] marvell-ccic: simplify and fix clk handling (a bit)
Hi Uwe,
Thanks for your reviewing. Please see the comments below.
>On Wed, Sep 25, 2013 at 07:47:19PM -0700, Libin Yang wrote:
>> In the clk enable and prepare function, we will check the NULL
>> pointer. So it should be no problem.
>I'm not sure what you mean here and unfortunately your quoting style makes
>your statement
>appear without context.
>
> if (... && !IS_ERR(cam->mipi_clk)) {
> if (cam->mipi_clk)
> devm_clk_put(mcam->dev, cam->mipi_clk);
> cam->mipi_clk = NULL;
> }
>
>might work in your setup, but it's wrong usage of the clk API. There is no
>reason NULL
>couldn't be a valid clk pointer. Moreover I cannot find a place in that driver
>that calls prepare
>and/or enable for the mipi_clk.
[Libin] Right. NULL could be a valid clk pointer. In the code, the clk will not
be released if mipi_clk is NULL.
Is below OK?
+ if (... && !IS_ERR(cam->mipi_clk)) {
+ devm_clk_put(mcam->dev, cam->mipi_clk);
+ cam->mipi_clk = NULL;
+ }
Set cam->mipi_clk = NULL is let cam->mipi_clk to be the initial state just like
after cam is allocated.
>(BTW, calling clk_get_rate on a disabled clk is another thing you shouldn't
>do.)
[Libin] Thanks for pointing it out. We enable the clk in other components.
Yes, you are right. We should enable the clk explicitly here.
>
>> For the mipi_clk, it is shared between other components, so we put the clk
>> it we don't use it.
>There should be no problem if >1 driver holds a reference to the clock. It's
>clk_disable (+
>maybe clk_unprepare) you should call when you don't need it any more. (But
>even having >1
>driver enabling a clk isn't a problem ...)
[Libin] So you mean we need not release the clk here and wait for devm to
release it later? I will check it with my colleagues to see whether they are OK
with this.
>
>> >> This patch fixes all but the last issue in this list. This patch
>> >> doesn't introduce new reasons for not building, but there are
>> >> already several bulid problems.
>> >
>> >Care to report those?
>Sure:
>
> CC drivers/media/platform/marvell-ccic/mmp-driver.o
> drivers/media/platform/marvell-ccic/mmp-driver.c: In function
> 'mmpcam_calc_dphy':
> drivers/media/platform/marvell-ccic/mmp-driver.c:264:15: error: 'struct
>mmp_camera_platform_data' has no member named 'dphy3_algo'
> drivers/media/platform/marvell-ccic/mmp-driver.c:265:7: error:
>'DPHY3_ALGO_PXA910' undeclared (first use in this function)
> drivers/media/platform/marvell-ccic/mmp-driver.c:265:7: note: each
> undeclared
>identifier is reported only once for each function it appears in
> drivers/media/platform/marvell-ccic/mmp-driver.c:269:8: error: 'struct
>mmp_camera_platform_data' has no member named 'dphy'
> drivers/media/platform/marvell-ccic/mmp-driver.c:270:17: error: 'struct
>mmp_camera_platform_data' has no member named 'lane_clk'
> drivers/media/platform/marvell-ccic/mmp-driver.c:271:16: error: 'struct
>mmp_camera_platform_data' has no member named 'lane_clk'
> drivers/media/platform/marvell-ccic/mmp-driver.c:273:7: error:
>'DPHY3_ALGO_PXA2128' undeclared (first use in this function)
> drivers/media/platform/marvell-ccic/mmp-driver.c:277:8: error: 'struct
>mmp_camera_platform_data' has no member named 'dphy'
> drivers/media/platform/marvell-ccic/mmp-driver.c:278:17: error: 'struct
>mmp_camera_platform_data' has no member named 'lane_clk'
> drivers/media/platform/marvell-ccic/mmp-driver.c:279:16: error: 'struct
>mmp_camera_platform_data' has no member named 'lane_clk'
> drivers/media/platform/marvell-ccic/mmp-driver.c:308:7: error: 'struct
>mmp_camera_platform_data' has no member named 'dphy'
> drivers/media/platform/marvell-ccic/mmp-driver.c:312:104: error: 'struct
>mmp_camera_platform_data' has no member named 'dphy'
> drivers/media/platform/marvell-ccic/mmp-driver.c:312:120: error: 'struct
>mmp_camera_platform_data' has no member named 'dphy'
> drivers/media/platform/marvell-ccic/mmp-driver.c:312:136: error: 'struct
>mmp_camera_platform_data' has no member named 'dphy'
> drivers/media/platform/marvell-ccic/mmp-driver.c: In function
> 'mmpcam_probe':
> drivers/media/platform/marvell-ccic/mmp-driver.c:385:24: error: 'struct
>mmp_camera_platf
RE: [PATCH] [media] marvell-ccic: drop free_irq for devm_request_irq allocated irq
Hello Wei Yongjun,
Thanks for finding the issue. It seems the last part code is missed when I
submitted the patch. Please see the patch we submitted before in [REVIEW PATCH
V4 07/12] [media] marvell-ccic: switch to resource managed allocation and
request.
The last part patch is missed:
@@ -474,18 +461,10 @@ out_free:
static int mmpcam_remove(struct mmp_camera *cam) {
struct mcam_camera *mcam = &cam->mcam;
- struct mmp_camera_platform_data *pdata;
mmpcam_remove_device(cam);
- free_irq(cam->irq, mcam);
mccic_shutdown(mcam);
mmpcam_power_down(mcam);
- pdata = cam->pdev->dev.platform_data;
- gpio_free(pdata->sensor_reset_gpio);
- gpio_free(pdata->sensor_power_gpio);
- iounmap(cam->power_regs);
- iounmap(mcam->regs);
- kfree(cam);
return 0;
}
It seems there was some mistake when Albert transferring the submit patch task
to me. And I did not notice the patch was wrong when submitting the patches. It
is my fault. I'm sorry for the inconvenient
Regards,
Libin
>-Original Message-
>From: linux-media-ow...@vger.kernel.org
>[mailto:linux-media-ow...@vger.kernel.org] On
>Behalf Of Jonathan Corbet
>Sent: Wednesday, September 25, 2013 3:12 PM
>To: Wei Yongjun
>Cc: m.che...@samsung.com; yongjun_...@trendmicro.com.cn; linux-
>me...@vger.kernel.org; Libin Yang
>Subject: Re: [PATCH] [media] marvell-ccic: drop free_irq for devm_request_irq
>allocated irq
>
>On Tue, 24 Sep 2013 10:35:50 +0800
>Wei Yongjun wrote:
>
>> irq allocated with devm_request_irq should not be freed using
>> free_irq, because doing so causes a dangling pointer, and a subsequent
>> double free.
>
>Makes sense.
>
>Acked-by: Jonathan Corbet
>
>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
--
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 v2] [media] marvell-ccic: simplify and fix clk handling (a bit)
Hi Jonathan & Uwe, In the clk enable and prepare function, we will check the NULL pointer. So it should be no problem. For the mipi_clk, it is shared between other components, so we put the clk it we don't use it. For the free_irq, it's my fault. Out before patch really removed this code together with gpio free It missed the last part of the original patch. Regards, Libin >-Original Message- >From: Jonathan Corbet [mailto:cor...@lwn.net] >Sent: Wednesday, September 25, 2013 3:15 PM >To: Uwe Kleine-König >Cc: Mauro Carvalho Chehab; linux-media@vger.kernel.org; linux-arm- >ker...@lists.infradead.org; Russell King; ker...@pengutronix.de; Libin Yang >Subject: Re: [PATCH v2] [media] marvell-ccic: simplify and fix clk handling (a >bit) > >On Tue, 24 Sep 2013 20:59:47 +0200 >Uwe Kleine-König wrote: > >> The marvell-ccic does several things wrong or ineffectively in the >> clock handling and it's usage of the devm_* stuff >> >> - it assumes clk_get doesn't return NULL >> - it explicitly calls devm_clk_put instead just keeping the reference >>during it's lifetime and let the driver core call it >> - it calls kfree, gpio_free and free_irq for resources it requested >>using devm_kzalloc, devm_gpio_request and devm_request_irq >>respectively. >> - it mixes devm_ and unmanaged resources which probably results in a >>race condition during remove > >OK, all of that stuff was added this time around by Libin; my understanding of >that particular >hardware is ... minimal. The basic idea of the patch seems sound. I do note, >though, that >you've changed the behavior of the driver somewhat. The MIPI clock is current >obtained at >power-up time and released on power-down; you've moved it to probe time >instead, and it's >held for the lifetime of the driver. >Perhaps that's even better, I don't know...Libin, what do you say on that? > >The free_irq() call is also removed by a patch previously submitted by Wei >Yongjun. > >> This patch fixes all but the last issue in this list. This patch >> doesn't introduce new reasons for not building, but there are already >> several bulid problems. > >Care to report those? > >Thanks, > >jon
RE: [PATCH v3 0/7] marvell-ccic: update ccic driver to support some features
Hi Jonathan, Do you have some comments? Regards, Libin >-Original Message- >From: Libin Yang [mailto:lby...@marvell.com] >Sent: Wednesday, July 03, 2013 1:56 PM >To: cor...@lwn.net; g.liakhovet...@gmx.de >Cc: linux-media@vger.kernel.org; albert.v.w...@gmail.com; Libin Yang >Subject: [PATCH v3 0/7] marvell-ccic: update ccic driver to support some >features > >The patch set adds some feature into the marvell ccic driver > >Patch 1: Support MIPI sensor >Patch 2: Support clock tree >Patch 3: reset ccic when stop streaming, which makes CCIC more stable >Patch 4: refine the mcam_set_contig_buffer function >Patch 5: add some new fmts to support >Patch 6: add SOF-EOF pair check to make the CCIC more stable >Patch 7: use resource managed allocation > >change log: >Patch 1: > fix the unlock issue > add some comments > remove int mipi_enabled in struct mmp_camera_platform_data > get "mipi" clk at first in mmpcam_power_up() >Patch 2: > Add mcam_deinit_clk function > Some changes in mcam_init_clk function >Patch 7: > A little adjustment based patch 2 change > >Libin Yang (7): > marvell-ccic: add MIPI support for marvell-ccic driver > marvell-ccic: add clock tree support for marvell-ccic driver > marvell-ccic: reset ccic phy when stop streaming for stability > marvell-ccic: refine mcam_set_contig_buffer function > marvell-ccic: add new formats support for marvell-ccic driver > marvell-ccic: add SOF / EOF pair check for marvell-ccic driver > marvell-ccic: switch to resource managed allocation and request > > drivers/media/platform/marvell-ccic/cafe-driver.c |4 +- > drivers/media/platform/marvell-ccic/mcam-core.c | 325 + > drivers/media/platform/marvell-ccic/mcam-core.h | 50 +++- > drivers/media/platform/marvell-ccic/mmp-driver.c | 274 ++--- > include/media/mmp-camera.h| 18 ++ > 5 files changed, 578 insertions(+), 93 deletions(-) > >-- >1.7.9.5 -- 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
[PATCH v3 7/7] marvell-ccic: switch to resource managed allocation and request
This patch switchs to resource managed allocation and request in mmp-driver.
It can remove free resource operations.
Signed-off-by: Albert Wang
Signed-off-by: Libin Yang
---
drivers/media/platform/marvell-ccic/mmp-driver.c | 60 --
1 file changed, 22 insertions(+), 38 deletions(-)
diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c
b/drivers/media/platform/marvell-ccic/mmp-driver.c
index 7503983..94f7e5a 100644
--- a/drivers/media/platform/marvell-ccic/mmp-driver.c
+++ b/drivers/media/platform/marvell-ccic/mmp-driver.c
@@ -368,7 +368,7 @@ static int mmpcam_probe(struct platform_device *pdev)
if (!pdata)
return -ENODEV;
- cam = kzalloc(sizeof(*cam), GFP_KERNEL);
+ cam = devm_kzalloc(&pdev->dev, sizeof(*cam), GFP_KERNEL);
if (cam == NULL)
return -ENOMEM;
cam->pdev = pdev;
@@ -399,15 +399,11 @@ static int mmpcam_probe(struct platform_device *pdev)
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (res == NULL) {
dev_err(&pdev->dev, "no iomem resource!\n");
- ret = -ENODEV;
- goto out_free;
- }
- mcam->regs = ioremap(res->start, resource_size(res));
- if (mcam->regs == NULL) {
- dev_err(&pdev->dev, "MMIO ioremap fail\n");
- ret = -ENODEV;
- goto out_free;
+ return -ENODEV;
}
+ mcam->regs = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(mcam->regs))
+ return PTR_ERR(mcam->regs);
/*
* Power/clock memory is elsewhere; get it too. Perhaps this
* should really be managed outside of this driver?
@@ -415,40 +411,37 @@ static int mmpcam_probe(struct platform_device *pdev)
res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
if (res == NULL) {
dev_err(&pdev->dev, "no power resource!\n");
- ret = -ENODEV;
- goto out_unmap1;
- }
- cam->power_regs = ioremap(res->start, resource_size(res));
- if (cam->power_regs == NULL) {
- dev_err(&pdev->dev, "power MMIO ioremap fail\n");
- ret = -ENODEV;
- goto out_unmap1;
+ return -ENODEV;
}
+ cam->power_regs = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(cam->power_regs))
+ return PTR_ERR(cam->power_regs);
/*
* Find the i2c adapter. This assumes, of course, that the
* i2c bus is already up and functioning.
*/
mcam->i2c_adapter = platform_get_drvdata(pdata->i2c_device);
if (mcam->i2c_adapter == NULL) {
- ret = -ENODEV;
dev_err(&pdev->dev, "No i2c adapter\n");
- goto out_unmap2;
+ return -ENODEV;
}
/*
* Sensor GPIO pins.
*/
- ret = gpio_request(pdata->sensor_power_gpio, "cam-power");
+ ret = devm_gpio_request(&pdev->dev, pdata->sensor_power_gpio,
+ "cam-power");
if (ret) {
dev_err(&pdev->dev, "Can't get sensor power gpio %d",
pdata->sensor_power_gpio);
- goto out_unmap2;
+ return ret;
}
gpio_direction_output(pdata->sensor_power_gpio, 0);
- ret = gpio_request(pdata->sensor_reset_gpio, "cam-reset");
+ ret = devm_gpio_request(&pdev->dev, pdata->sensor_reset_gpio,
+ "cam-reset");
if (ret) {
dev_err(&pdev->dev, "Can't get sensor reset gpio %d",
pdata->sensor_reset_gpio);
- goto out_gpio;
+ return ret;
}
gpio_direction_output(pdata->sensor_reset_gpio, 0);
@@ -459,10 +452,10 @@ static int mmpcam_probe(struct platform_device *pdev)
*/
ret = mmpcam_power_up(mcam);
if (ret)
- goto out_gpio2;
+ goto out_deinit_clk;
ret = mccic_register(mcam);
if (ret)
- goto out_pwdn;
+ goto out_power_down;
/*
* Finally, set up our IRQ now that the core is ready to
* deal with it.
@@ -473,8 +466,8 @@ static int mmpcam_probe(struct platform_device *pdev)
goto out_unregister;
}
cam->irq = res->start;
- ret = request_irq(cam->irq, mmpcam_irq, IRQF_SHARED,
- "mmp-camera", mcam);
+ ret = devm_request_irq(&pdev->dev, cam->irq, mmpcam_irq, IRQF_SHARED,
+
[PATCH v3 5/7] marvell-ccic: add new formats support for marvell-ccic driver
This patch adds some planar formats support for marvell-ccic.
Signed-off-by: Albert Wang
Signed-off-by: Libin Yang
Acked-by: Jonathan Corbet
---
drivers/media/platform/marvell-ccic/mcam-core.c | 192 +++
drivers/media/platform/marvell-ccic/mcam-core.h |6 +
2 files changed, 165 insertions(+), 33 deletions(-)
diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c
b/drivers/media/platform/marvell-ccic/mcam-core.c
index dbd0c6b..5b838c0 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.c
+++ b/drivers/media/platform/marvell-ccic/mcam-core.c
@@ -103,6 +103,7 @@ static struct mcam_format_struct {
__u8 *desc;
__u32 pixelformat;
int bpp; /* Bytes per pixel */
+ bool planar;
enum v4l2_mbus_pixelcode mbus_code;
} mcam_formats[] = {
{
@@ -110,24 +111,56 @@ static struct mcam_format_struct {
.pixelformat= V4L2_PIX_FMT_YUYV,
.mbus_code = V4L2_MBUS_FMT_YUYV8_2X8,
.bpp= 2,
+ .planar = false,
+ },
+ {
+ .desc = "UYVY 4:2:2",
+ .pixelformat= V4L2_PIX_FMT_UYVY,
+ .mbus_code = V4L2_MBUS_FMT_YUYV8_2X8,
+ .bpp= 2,
+ .planar = false,
+ },
+ {
+ .desc = "YUV 4:2:2 PLANAR",
+ .pixelformat= V4L2_PIX_FMT_YUV422P,
+ .mbus_code = V4L2_MBUS_FMT_YUYV8_2X8,
+ .bpp= 2,
+ .planar = true,
+ },
+ {
+ .desc = "YUV 4:2:0 PLANAR",
+ .pixelformat= V4L2_PIX_FMT_YUV420,
+ .mbus_code = V4L2_MBUS_FMT_YUYV8_2X8,
+ .bpp= 2,
+ .planar = true,
+ },
+ {
+ .desc = "YVU 4:2:0 PLANAR",
+ .pixelformat= V4L2_PIX_FMT_YVU420,
+ .mbus_code = V4L2_MBUS_FMT_YUYV8_2X8,
+ .bpp= 2,
+ .planar = true,
},
{
.desc = "RGB 444",
.pixelformat= V4L2_PIX_FMT_RGB444,
.mbus_code = V4L2_MBUS_FMT_RGB444_2X8_PADHI_LE,
.bpp= 2,
+ .planar = false,
},
{
.desc = "RGB 565",
.pixelformat= V4L2_PIX_FMT_RGB565,
.mbus_code = V4L2_MBUS_FMT_RGB565_2X8_LE,
.bpp= 2,
+ .planar = false,
},
{
.desc = "Raw RGB Bayer",
.pixelformat= V4L2_PIX_FMT_SBGGR8,
.mbus_code = V4L2_MBUS_FMT_SBGGR8_1X8,
- .bpp= 1
+ .bpp= 1,
+ .planar = false,
},
};
#define N_MCAM_FMTS ARRAY_SIZE(mcam_formats)
@@ -170,6 +203,12 @@ struct mcam_dma_desc {
u32 segment_len;
};
+struct yuv_pointer_t {
+ dma_addr_t y;
+ dma_addr_t u;
+ dma_addr_t v;
+};
+
/*
* Our buffer type for working with videobuf2. Note that the vb2
* developers have decreed that struct vb2_buffer must be at the
@@ -181,6 +220,7 @@ struct mcam_vb_buffer {
struct mcam_dma_desc *dma_desc; /* Descriptor virtual address */
dma_addr_t dma_desc_pa; /* Descriptor physical address */
int dma_desc_nent; /* Number of mapped descriptors */
+ struct yuv_pointer_t yuv_p;
};
static inline struct mcam_vb_buffer *vb_to_mvb(struct vb2_buffer *vb)
@@ -466,6 +506,15 @@ static inline int mcam_check_dma_buffers(struct
mcam_camera *cam)
/*
* DMA-contiguous code.
*/
+
+static bool mcam_fmt_is_planar(__u32 pfmt)
+{
+ struct mcam_format_struct *f;
+
+ f = mcam_find_format(pfmt);
+ return f->planar;
+}
+
/*
* Set up a contiguous buffer for the given frame. Here also is where
* the underrun strategy is set: if there is no buffer available, reuse
@@ -477,6 +526,11 @@ static inline int mcam_check_dma_buffers(struct
mcam_camera *cam)
static void mcam_set_contig_buffer(struct mcam_camera *cam, int frame)
{
struct mcam_vb_buffer *buf;
+ struct v4l2_pix_format *fmt = &cam->pix_format;
+ dma_addr_t dma_handle;
+ u32 pixel_count = fmt->width * fmt->height;
+ struct vb2_buffer *vb;
+
/*
* If there are no available buffers, go into single mode
*/
@@ -495,8 +549,35 @@ static void mcam_set_contig_buffer(struct mcam_camera
*cam, int frame)
}
cam->vb_bufs[frame] = buf;
- mcam_reg_write(cam, frame == 0 ? REG_Y0BAR : REG_Y1BAR,
- vb2_dma_contig_plane_dma_addr(&buf->vb_buf, 0));
+ vb =
[PATCH v3 1/7] marvell-ccic: add MIPI support for marvell-ccic driver
This patch adds the MIPI support for marvell-ccic.
Board driver should determine whether using MIPI or not.
Signed-off-by: Albert Wang
Signed-off-by: Libin Yang
---
drivers/media/platform/marvell-ccic/cafe-driver.c |4 +-
drivers/media/platform/marvell-ccic/mcam-core.c | 80 +++-
drivers/media/platform/marvell-ccic/mcam-core.h | 37 +-
drivers/media/platform/marvell-ccic/mmp-driver.c | 136 -
include/media/mmp-camera.h| 18 +++
5 files changed, 262 insertions(+), 13 deletions(-)
diff --git a/drivers/media/platform/marvell-ccic/cafe-driver.c
b/drivers/media/platform/marvell-ccic/cafe-driver.c
index d030f9b..68e82fb 100644
--- a/drivers/media/platform/marvell-ccic/cafe-driver.c
+++ b/drivers/media/platform/marvell-ccic/cafe-driver.c
@@ -400,7 +400,7 @@ static void cafe_ctlr_init(struct mcam_camera *mcam)
}
-static void cafe_ctlr_power_up(struct mcam_camera *mcam)
+static int cafe_ctlr_power_up(struct mcam_camera *mcam)
{
/*
* Part one of the sensor dance: turn the global
@@ -415,6 +415,8 @@ static void cafe_ctlr_power_up(struct mcam_camera *mcam)
*/
mcam_reg_write(mcam, REG_GPR, GPR_C1EN|GPR_C0EN); /* pwr up, reset */
mcam_reg_write(mcam, REG_GPR, GPR_C1EN|GPR_C0EN|GPR_C0);
+
+ return 0;
}
static void cafe_ctlr_power_down(struct mcam_camera *mcam)
diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c
b/drivers/media/platform/marvell-ccic/mcam-core.c
index 64ab91e..abe8538 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.c
+++ b/drivers/media/platform/marvell-ccic/mcam-core.c
@@ -19,6 +19,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -254,6 +255,45 @@ static void mcam_ctlr_stop(struct mcam_camera *cam)
mcam_reg_clear_bit(cam, REG_CTRL0, C0_ENABLE);
}
+static void mcam_enable_mipi(struct mcam_camera *mcam)
+{
+ /* Using MIPI mode and enable MIPI */
+ cam_dbg(mcam, "camera: DPHY3=0x%x, DPHY5=0x%x, DPHY6=0x%x\n",
+ mcam->dphy[0], mcam->dphy[1], mcam->dphy[2]);
+ mcam_reg_write(mcam, REG_CSI2_DPHY3, mcam->dphy[0]);
+ mcam_reg_write(mcam, REG_CSI2_DPHY5, mcam->dphy[1]);
+ mcam_reg_write(mcam, REG_CSI2_DPHY6, mcam->dphy[2]);
+
+ if (!mcam->mipi_enabled) {
+ if (mcam->lane > 4 || mcam->lane <= 0) {
+ cam_warn(mcam, "lane number error\n");
+ mcam->lane = 1; /* set the default value */
+ }
+ /*
+* 0x41 actives 1 lane
+* 0x43 actives 2 lanes
+* 0x45 actives 3 lanes (never happen)
+* 0x47 actives 4 lanes
+*/
+ mcam_reg_write(mcam, REG_CSI2_CTRL0,
+ CSI2_C0_MIPI_EN | CSI2_C0_ACT_LANE(mcam->lane));
+ mcam_reg_write(mcam, REG_CLKCTRL,
+ (mcam->mclk_src << 29) | mcam->mclk_div);
+
+ mcam->mipi_enabled = true;
+ }
+}
+
+static void mcam_disable_mipi(struct mcam_camera *mcam)
+{
+ /* Using Parallel mode or disable MIPI */
+ mcam_reg_write(mcam, REG_CSI2_CTRL0, 0x0);
+ mcam_reg_write(mcam, REG_CSI2_DPHY3, 0x0);
+ mcam_reg_write(mcam, REG_CSI2_DPHY5, 0x0);
+ mcam_reg_write(mcam, REG_CSI2_DPHY6, 0x0);
+ mcam->mipi_enabled = false;
+}
+
/* --- */
#ifdef MCAM_MODE_VMALLOC
@@ -657,6 +697,13 @@ static void mcam_ctlr_image(struct mcam_camera *cam)
*/
mcam_reg_write_mask(cam, REG_CTRL0, C0_SIF_HVSYNC,
C0_SIFM_MASK);
+
+ /*
+* This field controls the generation of EOF(DVP only)
+*/
+ if (cam->bus_type != V4L2_MBUS_CSI2)
+ mcam_reg_set_bit(cam, REG_CTRL0,
+ C0_EOF_VSYNC | C0_VEDGE_CTRL);
}
@@ -754,15 +801,21 @@ static void mcam_ctlr_stop_dma(struct mcam_camera *cam)
/*
* Power up and down.
*/
-static void mcam_ctlr_power_up(struct mcam_camera *cam)
+static int mcam_ctlr_power_up(struct mcam_camera *cam)
{
unsigned long flags;
+ int ret;
spin_lock_irqsave(&cam->dev_lock, flags);
- cam->plat_power_up(cam);
+ ret = cam->plat_power_up(cam);
+ if (ret) {
+ spin_unlock_irqrestore(&cam->dev_lock, flags);
+ return ret;
+ }
mcam_reg_clear_bit(cam, REG_CTRL1, C1_PWRDWN);
spin_unlock_irqrestore(&cam->dev_lock, flags);
msleep(5); /* Just to be sure */
+ return 0;
}
static void mcam_ctlr_power_down(struct mcam_camera *cam)
@@ -887,6 +940,17 @@ static int mcam_read_setup(struct mcam_camera *cam)
spin_lock_irqsave(&cam->dev_lock, flags);
clear_bit(CF_DMA_ACTIVE, &cam->fla
[PATCH v3 6/7] marvell-ccic: add SOF / EOF pair check for marvell-ccic driver
This patch adds the SOFx/EOFx pair check for marvell-ccic.
When switching format, the last EOF may not arrive when stop streamning.
And the EOF will be detected in the next start streaming.
Must ensure clear the left over frame flags before every really start streaming.
Signed-off-by: Albert Wang
Signed-off-by: Libin Yang
Acked-by: Jonathan Corbet
Acked-by: Guennadi Liakhovetski
---
drivers/media/platform/marvell-ccic/mcam-core.c | 30 ---
1 file changed, 26 insertions(+), 4 deletions(-)
diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c
b/drivers/media/platform/marvell-ccic/mcam-core.c
index 5b838c0..f3a260c 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.c
+++ b/drivers/media/platform/marvell-ccic/mcam-core.c
@@ -95,6 +95,9 @@ MODULE_PARM_DESC(buffer_mode,
#define CF_CONFIG_NEEDED 4 /* Must configure hardware */
#define CF_SINGLE_BUFFER 5 /* Running with a single buffer */
#define CF_SG_RESTART 6 /* SG restart needed */
+#define CF_FRAME_SOF0 7 /* Frame 0 started */
+#define CF_FRAME_SOF1 8
+#define CF_FRAME_SOF2 9
#define sensor_call(cam, o, f, args...) \
v4l2_subdev_call(cam->sensor, o, f, ##args)
@@ -261,8 +264,10 @@ static void mcam_reset_buffers(struct mcam_camera *cam)
int i;
cam->next_buf = -1;
- for (i = 0; i < cam->nbufs; i++)
+ for (i = 0; i < cam->nbufs; i++) {
clear_bit(i, &cam->flags);
+ clear_bit(CF_FRAME_SOF0 + i, &cam->flags);
+ }
}
static inline int mcam_needs_config(struct mcam_camera *cam)
@@ -1140,6 +1145,7 @@ static void mcam_vb_wait_finish(struct vb2_queue *vq)
static int mcam_vb_start_streaming(struct vb2_queue *vq, unsigned int count)
{
struct mcam_camera *cam = vb2_get_drv_priv(vq);
+ unsigned int frame;
if (cam->state != S_IDLE) {
INIT_LIST_HEAD(&cam->buffers);
@@ -1157,6 +1163,14 @@ static int mcam_vb_start_streaming(struct vb2_queue *vq,
unsigned int count)
cam->state = S_BUFWAIT;
return 0;
}
+
+ /*
+* Ensure clear the left over frame flags
+* before every really start streaming
+*/
+ for (frame = 0; frame < cam->nbufs; frame++)
+ clear_bit(CF_FRAME_SOF0 + frame, &cam->flags);
+
return mcam_read_setup(cam);
}
@@ -1845,9 +1859,11 @@ int mccic_irq(struct mcam_camera *cam, unsigned int irqs)
* each time.
*/
for (frame = 0; frame < cam->nbufs; frame++)
- if (irqs & (IRQ_EOF0 << frame)) {
+ if (irqs & (IRQ_EOF0 << frame) &&
+ test_bit(CF_FRAME_SOF0 + frame, &cam->flags)) {
mcam_frame_complete(cam, frame);
handled = 1;
+ clear_bit(CF_FRAME_SOF0 + frame, &cam->flags);
if (cam->buffer_mode == B_DMA_sg)
break;
}
@@ -1856,9 +1872,15 @@ int mccic_irq(struct mcam_camera *cam, unsigned int irqs)
* code assumes that we won't get multiple frame interrupts
* at once; may want to rethink that.
*/
- if (irqs & (IRQ_SOF0 | IRQ_SOF1 | IRQ_SOF2)) {
+ for (frame = 0; frame < cam->nbufs; frame++) {
+ if (irqs & (IRQ_SOF0 << frame)) {
+ set_bit(CF_FRAME_SOF0 + frame, &cam->flags);
+ handled = IRQ_HANDLED;
+ }
+ }
+
+ if (handled == IRQ_HANDLED) {
set_bit(CF_DMA_ACTIVE, &cam->flags);
- handled = 1;
if (cam->buffer_mode == B_DMA_sg)
mcam_ctlr_stop(cam);
}
--
1.7.9.5
--
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
[PATCH v3 0/7] marvell-ccic: update ccic driver to support some features
The patch set adds some feature into the marvell ccic driver Patch 1: Support MIPI sensor Patch 2: Support clock tree Patch 3: reset ccic when stop streaming, which makes CCIC more stable Patch 4: refine the mcam_set_contig_buffer function Patch 5: add some new fmts to support Patch 6: add SOF-EOF pair check to make the CCIC more stable Patch 7: use resource managed allocation change log: Patch 1: fix the unlock issue add some comments remove int mipi_enabled in struct mmp_camera_platform_data get "mipi" clk at first in mmpcam_power_up() Patch 2: Add mcam_deinit_clk function Some changes in mcam_init_clk function Patch 7: A little adjustment based patch 2 change Libin Yang (7): marvell-ccic: add MIPI support for marvell-ccic driver marvell-ccic: add clock tree support for marvell-ccic driver marvell-ccic: reset ccic phy when stop streaming for stability marvell-ccic: refine mcam_set_contig_buffer function marvell-ccic: add new formats support for marvell-ccic driver marvell-ccic: add SOF / EOF pair check for marvell-ccic driver marvell-ccic: switch to resource managed allocation and request drivers/media/platform/marvell-ccic/cafe-driver.c |4 +- drivers/media/platform/marvell-ccic/mcam-core.c | 325 + drivers/media/platform/marvell-ccic/mcam-core.h | 50 +++- drivers/media/platform/marvell-ccic/mmp-driver.c | 274 ++--- include/media/mmp-camera.h| 18 ++ 5 files changed, 578 insertions(+), 93 deletions(-) -- 1.7.9.5 -- 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
[PATCH v3 4/7] marvell-ccic: refine mcam_set_contig_buffer function
This patch refines mcam_set_contig_buffer() in mcam core.
It can remove redundant code line and enhance readability.
Signed-off-by: Albert Wang
Signed-off-by: Libin Yang
Acked-by: Guennadi Liakhovetski
Acked-by: Jonathan Corbet
---
drivers/media/platform/marvell-ccic/mcam-core.c | 21 ++---
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c
b/drivers/media/platform/marvell-ccic/mcam-core.c
index 6921e84..dbd0c6b 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.c
+++ b/drivers/media/platform/marvell-ccic/mcam-core.c
@@ -482,22 +482,21 @@ static void mcam_set_contig_buffer(struct mcam_camera
*cam, int frame)
*/
if (list_empty(&cam->buffers)) {
buf = cam->vb_bufs[frame ^ 0x1];
- cam->vb_bufs[frame] = buf;
- mcam_reg_write(cam, frame == 0 ? REG_Y0BAR : REG_Y1BAR,
- vb2_dma_contig_plane_dma_addr(&buf->vb_buf, 0));
set_bit(CF_SINGLE_BUFFER, &cam->flags);
cam->frame_state.singles++;
- return;
+ } else {
+ /*
+* OK, we have a buffer we can use.
+*/
+ buf = list_first_entry(&cam->buffers, struct mcam_vb_buffer,
+ queue);
+ list_del_init(&buf->queue);
+ clear_bit(CF_SINGLE_BUFFER, &cam->flags);
}
- /*
-* OK, we have a buffer we can use.
-*/
- buf = list_first_entry(&cam->buffers, struct mcam_vb_buffer, queue);
- list_del_init(&buf->queue);
+
+ cam->vb_bufs[frame] = buf;
mcam_reg_write(cam, frame == 0 ? REG_Y0BAR : REG_Y1BAR,
vb2_dma_contig_plane_dma_addr(&buf->vb_buf, 0));
- cam->vb_bufs[frame] = buf;
- clear_bit(CF_SINGLE_BUFFER, &cam->flags);
}
/*
--
1.7.9.5
--
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
[PATCH v3 2/7] marvell-ccic: add clock tree support for marvell-ccic driver
This patch adds the clock tree support for marvell-ccic.
Signed-off-by: Libin Yang
Signed-off-by: Albert Wang
---
drivers/media/platform/marvell-ccic/mcam-core.h |5 ++
drivers/media/platform/marvell-ccic/mmp-driver.c | 61 ++
2 files changed, 66 insertions(+)
diff --git a/drivers/media/platform/marvell-ccic/mcam-core.h
b/drivers/media/platform/marvell-ccic/mcam-core.h
index 26fb154..07fdf68 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.h
+++ b/drivers/media/platform/marvell-ccic/mcam-core.h
@@ -83,6 +83,8 @@ struct mcam_frame_state {
unsigned int delivered;
};
+#define NR_MCAM_CLK 3
+
/*
* A description of one of our devices.
* Locking: controlled by s_mutex. Certain fields, however, require
@@ -118,6 +120,9 @@ struct mcam_camera {
bool mipi_enabled; /* flag whether mipi is enabled already */
int lane; /* lane number */
+ /* clock tree support */
+ struct clk *clk[NR_MCAM_CLK];
+
/*
* Callbacks from the core to the platform code.
*/
diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c
b/drivers/media/platform/marvell-ccic/mmp-driver.c
index 2b28802..64dacc5 100644
--- a/drivers/media/platform/marvell-ccic/mmp-driver.c
+++ b/drivers/media/platform/marvell-ccic/mmp-driver.c
@@ -35,6 +35,8 @@ MODULE_ALIAS("platform:mmp-camera");
MODULE_AUTHOR("Jonathan Corbet ");
MODULE_LICENSE("GPL");
+static char *mcam_clks[] = {"CCICAXICLK", "CCICFUNCLK", "CCICPHYCLK"};
+
struct mmp_camera {
void *power_regs;
struct platform_device *pdev;
@@ -105,6 +107,26 @@ static struct mmp_camera *mmpcam_find_device(struct
platform_device *pdev)
#define REG_CCIC_DCGCR 0x28/* CCIC dyn clock gate ctrl reg */
#define REG_CCIC_CRCR 0x50/* CCIC clk reset ctrl reg */
+static void mcam_clk_enable(struct mcam_camera *mcam)
+{
+ unsigned int i;
+
+ for (i = 0; i < NR_MCAM_CLK; i++) {
+ if (!IS_ERR(mcam->clk[i]))
+ clk_prepare_enable(mcam->clk[i]);
+ }
+}
+
+static void mcam_clk_disable(struct mcam_camera *mcam)
+{
+ int i;
+
+ for (i = NR_MCAM_CLK - 1; i >= 0; i--) {
+ if (!IS_ERR(mcam->clk[i]))
+ clk_disable_unprepare(mcam->clk[i]);
+ }
+}
+
/*
* Power control.
*/
@@ -142,6 +164,9 @@ static int mmpcam_power_up(struct mcam_camera *mcam)
mdelay(5);
gpio_set_value(pdata->sensor_reset_gpio, 1); /* reset is active low */
mdelay(5);
+
+ mcam_clk_enable(mcam);
+
return 0;
}
@@ -166,6 +191,8 @@ static void mmpcam_power_down(struct mcam_camera *mcam)
devm_clk_put(mcam->dev, cam->mipi_clk);
cam->mipi_clk = NULL;
}
+
+ mcam_clk_disable(mcam);
}
/*
@@ -276,6 +303,35 @@ static irqreturn_t mmpcam_irq(int irq, void *data)
return IRQ_RETVAL(handled);
}
+static void mcam_deinit_clk(struct mcam_camera *mcam)
+{
+ unsigned int i;
+
+ for (i = 0; i < NR_MCAM_CLK; i++) {
+ if (!IS_ERR(mcam->clk[i])) {
+ if (mcam->clk[i])
+ devm_clk_put(mcam->dev, mcam->clk[i]);
+ }
+ mcam->clk[i] = NULL;
+ }
+}
+
+static void mcam_init_clk(struct mcam_camera *mcam)
+{
+ unsigned int i;
+
+ for (i = 0; i < NR_MCAM_CLK; i++) {
+ if (mcam_clks[i] != NULL) {
+ /* Some clks are not necessary on some boards
+* We still try to run even it fails getting clk
+*/
+ mcam->clk[i] = devm_clk_get(mcam->dev, mcam_clks[i]);
+ if (IS_ERR(mcam->clk[i]))
+ dev_warn(mcam->dev, "Could not get clk: %s\n",
+ mcam_clks[i]);
+ }
+ }
+}
static int mmpcam_probe(struct platform_device *pdev)
{
@@ -370,6 +426,9 @@ static int mmpcam_probe(struct platform_device *pdev)
goto out_gpio;
}
gpio_direction_output(pdata->sensor_reset_gpio, 0);
+
+ mcam_init_clk(mcam);
+
/*
* Power the device up and hand it off to the core.
*/
@@ -401,6 +460,7 @@ out_unregister:
out_pwdn:
mmpcam_power_down(mcam);
out_gpio2:
+ mcam_deinit_clk(mcam);
gpio_free(pdata->sensor_reset_gpio);
out_gpio:
gpio_free(pdata->sensor_power_gpio);
@@ -426,6 +486,7 @@ static int mmpcam_remove(struct mmp_camera *cam)
pdata = cam->pdev->dev.platform_data;
gpio_free(pdata->sensor_reset_gpio);
gpio_free(pdata->sensor_power_gpio);
+ mcam_deinit_clk(mcam);
iounmap(cam->power_regs);
iounm
[PATCH v3 3/7] marvell-ccic: reset ccic phy when stop streaming for stability
This patch adds the reset ccic phy operation when stop streaming.
Stop streaming without reset ccic phy, the next start streaming
may be unstable.
Also need add CCIC2 definition when PXA688/PXA2128 support dual ccics.
Signed-off-by: Albert Wang
Signed-off-by: Libin Yang
Acked-by: Jonathan Corbet
Acked-by: Guennadi Liakhovetski
---
drivers/media/platform/marvell-ccic/mcam-core.c |6 ++
drivers/media/platform/marvell-ccic/mcam-core.h |2 ++
drivers/media/platform/marvell-ccic/mmp-driver.c | 25 ++
3 files changed, 33 insertions(+)
diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c
b/drivers/media/platform/marvell-ccic/mcam-core.c
index abe8538..6921e84 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.c
+++ b/drivers/media/platform/marvell-ccic/mcam-core.c
@@ -1059,6 +1059,12 @@ static int mcam_vb_stop_streaming(struct vb2_queue *vq)
return -EINVAL;
mcam_ctlr_stop_dma(cam);
/*
+* Reset the CCIC PHY after stopping streaming,
+* otherwise, the CCIC may be unstable.
+*/
+ if (cam->ctlr_reset)
+ cam->ctlr_reset(cam);
+ /*
* VB2 reclaims the buffers, so we need to forget
* about them.
*/
diff --git a/drivers/media/platform/marvell-ccic/mcam-core.h
b/drivers/media/platform/marvell-ccic/mcam-core.h
index 07fdf68..4d4db6b 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.h
+++ b/drivers/media/platform/marvell-ccic/mcam-core.h
@@ -109,6 +109,7 @@ struct mcam_camera {
int mclk_src; /* which clock source the mclk derives from */
int mclk_div; /* Clock Divider Value for MCLK */
+ int ccic_id;
enum v4l2_mbus_type bus_type;
/* MIPI support */
/* The dphy config value, allocated in board file
@@ -129,6 +130,7 @@ struct mcam_camera {
int (*plat_power_up) (struct mcam_camera *cam);
void (*plat_power_down) (struct mcam_camera *cam);
void (*calc_dphy) (struct mcam_camera *cam);
+ void (*ctlr_reset) (struct mcam_camera *cam);
/*
* Everything below here is private to the mcam core and
diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c
b/drivers/media/platform/marvell-ccic/mmp-driver.c
index 64dacc5..7503983 100644
--- a/drivers/media/platform/marvell-ccic/mmp-driver.c
+++ b/drivers/media/platform/marvell-ccic/mmp-driver.c
@@ -106,6 +106,7 @@ static struct mmp_camera *mmpcam_find_device(struct
platform_device *pdev)
#define CPU_SUBSYS_PMU_BASE0xd4282800
#define REG_CCIC_DCGCR 0x28/* CCIC dyn clock gate ctrl reg */
#define REG_CCIC_CRCR 0x50/* CCIC clk reset ctrl reg */
+#define REG_CCIC2_CRCR 0xf4/* CCIC2 clk reset ctrl reg */
static void mcam_clk_enable(struct mcam_camera *mcam)
{
@@ -195,6 +196,28 @@ static void mmpcam_power_down(struct mcam_camera *mcam)
mcam_clk_disable(mcam);
}
+void mcam_ctlr_reset(struct mcam_camera *mcam)
+{
+ unsigned long val;
+ struct mmp_camera *cam = mcam_to_cam(mcam);
+
+ if (mcam->ccic_id) {
+ /*
+* Using CCIC2
+*/
+ val = ioread32(cam->power_regs + REG_CCIC2_CRCR);
+ iowrite32(val & ~0x2, cam->power_regs + REG_CCIC2_CRCR);
+ iowrite32(val | 0x2, cam->power_regs + REG_CCIC2_CRCR);
+ } else {
+ /*
+* Using CCIC1
+*/
+ val = ioread32(cam->power_regs + REG_CCIC_CRCR);
+ iowrite32(val & ~0x2, cam->power_regs + REG_CCIC_CRCR);
+ iowrite32(val | 0x2, cam->power_regs + REG_CCIC_CRCR);
+ }
+}
+
/*
* calc the dphy register values
* There are three dphy registers being used.
@@ -355,9 +378,11 @@ static int mmpcam_probe(struct platform_device *pdev)
mcam = &cam->mcam;
mcam->plat_power_up = mmpcam_power_up;
mcam->plat_power_down = mmpcam_power_down;
+ mcam->ctlr_reset = mcam_ctlr_reset;
mcam->calc_dphy = mmpcam_calc_dphy;
mcam->dev = &pdev->dev;
mcam->use_smbus = 0;
+ mcam->ccic_id = pdev->id;
mcam->mclk_min = pdata->mclk_min;
mcam->mclk_src = pdata->mclk_src;
mcam->mclk_div = pdata->mclk_div;
--
1.7.9.5
--
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 v2 1/7] marvell-ccic: add MIPI support for marvell-ccic driver
Hi Jonathan,
Sorry, it's my silly fault. I will update it.
Regards,
Libin
>-Original Message-
>From: Jonathan Corbet [mailto:cor...@lwn.net]
>Sent: Tuesday, July 02, 2013 12:08 PM
>To: Libin Yang
>Cc: g.liakhovet...@gmx.de; linux-media@vger.kernel.org;
>albert.v.w...@gmail.com;
>Albert Wang
>Subject: Re: [PATCH v2 1/7] marvell-ccic: add MIPI support for marvell-ccic
>driver
>
>For future reference, it's nice to summarize the changes made since the
>previous posting.
>
>In a really quick scan, I immediately stumbled across this:
>
>> @@ -1816,7 +1884,9 @@ int mccic_resume(struct mcam_camera *cam)
>>
>> mutex_lock(&cam->s_mutex);
>> if (cam->users > 0) {
>> -mcam_ctlr_power_up(cam);
>> +ret = mcam_ctlr_power_up(cam);
>> +if (ret)
>> +return ret;
>
>You do see the problem here, right? Can I ask you to please audit *all*
>of your changes to be sure they don't leak locks? This isn't the sort of
>problem that should need to be pointed out twice.
>
>Don't get me wrong, I very much appreciate the effort you have put into
>getting these patches ready for merging, and things are quite close. But
>it's important to get details like this right.
>
>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
[PATCH v2 7/7] marvell-ccic: switch to resource managed allocation and request
This patch switchs to resource managed allocation and request in mmp-driver.
It can remove free resource operations.
Signed-off-by: Albert Wang
Signed-off-by: Libin Yang
Acked-by: Jonathan Corbet
---
drivers/media/platform/marvell-ccic/mmp-driver.c | 68 --
1 file changed, 23 insertions(+), 45 deletions(-)
diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c
b/drivers/media/platform/marvell-ccic/mmp-driver.c
index 55fd47b..d862507 100644
--- a/drivers/media/platform/marvell-ccic/mmp-driver.c
+++ b/drivers/media/platform/marvell-ccic/mmp-driver.c
@@ -354,7 +354,7 @@ static int mmpcam_probe(struct platform_device *pdev)
if (!pdata)
return -ENODEV;
- cam = kzalloc(sizeof(*cam), GFP_KERNEL);
+ cam = devm_kzalloc(&pdev->dev, sizeof(*cam), GFP_KERNEL);
if (cam == NULL)
return -ENOMEM;
cam->pdev = pdev;
@@ -385,15 +385,11 @@ static int mmpcam_probe(struct platform_device *pdev)
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (res == NULL) {
dev_err(&pdev->dev, "no iomem resource!\n");
- ret = -ENODEV;
- goto out_free;
- }
- mcam->regs = ioremap(res->start, resource_size(res));
- if (mcam->regs == NULL) {
- dev_err(&pdev->dev, "MMIO ioremap fail\n");
- ret = -ENODEV;
- goto out_free;
+ return -ENODEV;
}
+ mcam->regs = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(mcam->regs))
+ return PTR_ERR(mcam->regs);
/*
* Power/clock memory is elsewhere; get it too. Perhaps this
* should really be managed outside of this driver?
@@ -401,15 +397,11 @@ static int mmpcam_probe(struct platform_device *pdev)
res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
if (res == NULL) {
dev_err(&pdev->dev, "no power resource!\n");
- ret = -ENODEV;
- goto out_unmap1;
- }
- cam->power_regs = ioremap(res->start, resource_size(res));
- if (cam->power_regs == NULL) {
- dev_err(&pdev->dev, "power MMIO ioremap fail\n");
- ret = -ENODEV;
- goto out_unmap1;
+ return -ENODEV;
}
+ cam->power_regs = devm_ioremap_resource(&pdev->dev, res);
+ if (IS_ERR(cam->power_regs))
+ return PTR_ERR(cam->power_regs);
mcam_init_clk(mcam, pdata);
@@ -419,25 +411,28 @@ static int mmpcam_probe(struct platform_device *pdev)
*/
mcam->i2c_adapter = platform_get_drvdata(pdata->i2c_device);
if (mcam->i2c_adapter == NULL) {
- ret = -ENODEV;
dev_err(&pdev->dev, "No i2c adapter\n");
- goto out_unmap2;
+ return -ENODEV;
}
/*
* Sensor GPIO pins.
*/
- ret = gpio_request(pdata->sensor_power_gpio, "cam-power");
+ ret = devm_gpio_request(&pdev->dev, pdata->sensor_power_gpio,
+ "cam-power");
+
if (ret) {
dev_err(&pdev->dev, "Can't get sensor power gpio %d",
pdata->sensor_power_gpio);
- goto out_unmap2;
+ return ret;
}
gpio_direction_output(pdata->sensor_power_gpio, 0);
- ret = gpio_request(pdata->sensor_reset_gpio, "cam-reset");
+ ret = devm_gpio_request(&pdev->dev, pdata->sensor_reset_gpio,
+ "cam-reset");
+
if (ret) {
dev_err(&pdev->dev, "Can't get sensor reset gpio %d",
pdata->sensor_reset_gpio);
- goto out_gpio;
+ return ret;
}
gpio_direction_output(pdata->sensor_reset_gpio, 0);
/*
@@ -445,10 +440,10 @@ static int mmpcam_probe(struct platform_device *pdev)
*/
ret = mmpcam_power_up(mcam);
if (ret)
- goto out_gpio2;
+ goto out_power_down;
ret = mccic_register(mcam);
if (ret)
- goto out_gpio2;
+ goto out_power_down;
/*
* Finally, set up our IRQ now that the core is ready to
* deal with it.
@@ -459,8 +454,8 @@ static int mmpcam_probe(struct platform_device *pdev)
goto out_unregister;
}
cam->irq = res->start;
- ret = request_irq(cam->irq, mmpcam_irq, IRQF_SHARED,
- "mmp-camera", mcam);
+ ret = devm_request_irq(&pdev->dev, cam->irq, mmpcam_irq, IRQF_SHARE
[PATCH v2 0/7] marvell-ccic: update ccic driver to support some features
The patch set adds some feature into the marvell ccic driver Patch 1: Support MIPI sensor Patch 2: Support clock tree Patch 3: reset ccic when stop streaming, which makes CCIC more stable Patch 4: refine the mcam_set_contig_buffer function Patch 5: add some new fmts to support Patch 6: add SOF-EOF pair check to make the CCIC more stable Patch 7: use resource managed allocation Libin Yang (7): marvell-ccic: add MIPI support for marvell-ccic driver marvell-ccic: add clock tree support for marvell-ccic driver marvell-ccic: reset ccic phy when stop streaming for stability marvell-ccic: refine mcam_set_contig_buffer function marvell-ccic: add new formats support for marvell-ccic driver marvell-ccic: add SOF / EOF pair check for marvell-ccic driver marvell-ccic: switch to resource managed allocation and request drivers/media/platform/marvell-ccic/cafe-driver.c |4 +- drivers/media/platform/marvell-ccic/mcam-core.c | 323 + drivers/media/platform/marvell-ccic/mcam-core.h | 51 +++- drivers/media/platform/marvell-ccic/mmp-driver.c | 267 ++--- include/media/mmp-camera.h| 19 ++ 5 files changed, 563 insertions(+), 101 deletions(-) -- 1.7.9.5 -- 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
[PATCH v2 5/7] marvell-ccic: add new formats support for marvell-ccic driver
This patch adds some planar formats support for marvell-ccic.
Signed-off-by: Albert Wang
Signed-off-by: Libin Yang
Acked-by: Jonathan Corbet
---
drivers/media/platform/marvell-ccic/mcam-core.c | 192 +++
drivers/media/platform/marvell-ccic/mcam-core.h |6 +
2 files changed, 165 insertions(+), 33 deletions(-)
diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c
b/drivers/media/platform/marvell-ccic/mcam-core.c
index 160969d..7a2855f 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.c
+++ b/drivers/media/platform/marvell-ccic/mcam-core.c
@@ -103,6 +103,7 @@ static struct mcam_format_struct {
__u8 *desc;
__u32 pixelformat;
int bpp; /* Bytes per pixel */
+ bool planar;
enum v4l2_mbus_pixelcode mbus_code;
} mcam_formats[] = {
{
@@ -110,24 +111,56 @@ static struct mcam_format_struct {
.pixelformat= V4L2_PIX_FMT_YUYV,
.mbus_code = V4L2_MBUS_FMT_YUYV8_2X8,
.bpp= 2,
+ .planar = false,
+ },
+ {
+ .desc = "UYVY 4:2:2",
+ .pixelformat= V4L2_PIX_FMT_UYVY,
+ .mbus_code = V4L2_MBUS_FMT_YUYV8_2X8,
+ .bpp= 2,
+ .planar = false,
+ },
+ {
+ .desc = "YUV 4:2:2 PLANAR",
+ .pixelformat= V4L2_PIX_FMT_YUV422P,
+ .mbus_code = V4L2_MBUS_FMT_YUYV8_2X8,
+ .bpp= 2,
+ .planar = true,
+ },
+ {
+ .desc = "YUV 4:2:0 PLANAR",
+ .pixelformat= V4L2_PIX_FMT_YUV420,
+ .mbus_code = V4L2_MBUS_FMT_YUYV8_2X8,
+ .bpp= 2,
+ .planar = true,
+ },
+ {
+ .desc = "YVU 4:2:0 PLANAR",
+ .pixelformat= V4L2_PIX_FMT_YVU420,
+ .mbus_code = V4L2_MBUS_FMT_YUYV8_2X8,
+ .bpp= 2,
+ .planar = true,
},
{
.desc = "RGB 444",
.pixelformat= V4L2_PIX_FMT_RGB444,
.mbus_code = V4L2_MBUS_FMT_RGB444_2X8_PADHI_LE,
.bpp= 2,
+ .planar = false,
},
{
.desc = "RGB 565",
.pixelformat= V4L2_PIX_FMT_RGB565,
.mbus_code = V4L2_MBUS_FMT_RGB565_2X8_LE,
.bpp= 2,
+ .planar = false,
},
{
.desc = "Raw RGB Bayer",
.pixelformat= V4L2_PIX_FMT_SBGGR8,
.mbus_code = V4L2_MBUS_FMT_SBGGR8_1X8,
- .bpp= 1
+ .bpp= 1,
+ .planar = false,
},
};
#define N_MCAM_FMTS ARRAY_SIZE(mcam_formats)
@@ -170,6 +203,12 @@ struct mcam_dma_desc {
u32 segment_len;
};
+struct yuv_pointer_t {
+ dma_addr_t y;
+ dma_addr_t u;
+ dma_addr_t v;
+};
+
/*
* Our buffer type for working with videobuf2. Note that the vb2
* developers have decreed that struct vb2_buffer must be at the
@@ -181,6 +220,7 @@ struct mcam_vb_buffer {
struct mcam_dma_desc *dma_desc; /* Descriptor virtual address */
dma_addr_t dma_desc_pa; /* Descriptor physical address */
int dma_desc_nent; /* Number of mapped descriptors */
+ struct yuv_pointer_t yuv_p;
};
static inline struct mcam_vb_buffer *vb_to_mvb(struct vb2_buffer *vb)
@@ -466,6 +506,15 @@ static inline int mcam_check_dma_buffers(struct
mcam_camera *cam)
/*
* DMA-contiguous code.
*/
+
+static bool mcam_fmt_is_planar(__u32 pfmt)
+{
+ struct mcam_format_struct *f;
+
+ f = mcam_find_format(pfmt);
+ return f->planar;
+}
+
/*
* Set up a contiguous buffer for the given frame. Here also is where
* the underrun strategy is set: if there is no buffer available, reuse
@@ -477,6 +526,11 @@ static inline int mcam_check_dma_buffers(struct
mcam_camera *cam)
static void mcam_set_contig_buffer(struct mcam_camera *cam, int frame)
{
struct mcam_vb_buffer *buf;
+ struct v4l2_pix_format *fmt = &cam->pix_format;
+ dma_addr_t dma_handle;
+ u32 pixel_count = fmt->width * fmt->height;
+ struct vb2_buffer *vb;
+
/*
* If there are no available buffers, go into single mode
*/
@@ -495,8 +549,35 @@ static void mcam_set_contig_buffer(struct mcam_camera
*cam, int frame)
}
cam->vb_bufs[frame] = buf;
- mcam_reg_write(cam, frame == 0 ? REG_Y0BAR : REG_Y1BAR,
- vb2_dma_contig_plane_dma_addr(&buf->vb_buf, 0));
+ vb =
[PATCH v2 4/7] marvell-ccic: refine mcam_set_contig_buffer function
This patch refines mcam_set_contig_buffer() in mcam core.
It can remove redundant code line and enhance readability.
Signed-off-by: Albert Wang
Signed-off-by: Libin Yang
Acked-by: Guennadi Liakhovetski
Acked-by: Jonathan Corbet
---
drivers/media/platform/marvell-ccic/mcam-core.c | 21 ++---
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c
b/drivers/media/platform/marvell-ccic/mcam-core.c
index 61dd5be..160969d 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.c
+++ b/drivers/media/platform/marvell-ccic/mcam-core.c
@@ -482,22 +482,21 @@ static void mcam_set_contig_buffer(struct mcam_camera
*cam, int frame)
*/
if (list_empty(&cam->buffers)) {
buf = cam->vb_bufs[frame ^ 0x1];
- cam->vb_bufs[frame] = buf;
- mcam_reg_write(cam, frame == 0 ? REG_Y0BAR : REG_Y1BAR,
- vb2_dma_contig_plane_dma_addr(&buf->vb_buf, 0));
set_bit(CF_SINGLE_BUFFER, &cam->flags);
cam->frame_state.singles++;
- return;
+ } else {
+ /*
+* OK, we have a buffer we can use.
+*/
+ buf = list_first_entry(&cam->buffers, struct mcam_vb_buffer,
+ queue);
+ list_del_init(&buf->queue);
+ clear_bit(CF_SINGLE_BUFFER, &cam->flags);
}
- /*
-* OK, we have a buffer we can use.
-*/
- buf = list_first_entry(&cam->buffers, struct mcam_vb_buffer, queue);
- list_del_init(&buf->queue);
+
+ cam->vb_bufs[frame] = buf;
mcam_reg_write(cam, frame == 0 ? REG_Y0BAR : REG_Y1BAR,
vb2_dma_contig_plane_dma_addr(&buf->vb_buf, 0));
- cam->vb_bufs[frame] = buf;
- clear_bit(CF_SINGLE_BUFFER, &cam->flags);
}
/*
--
1.7.9.5
--
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
[PATCH v2 6/7] marvell-ccic: add SOF / EOF pair check for marvell-ccic driver
This patch adds the SOFx/EOFx pair check for marvell-ccic.
When switching format, the last EOF may not arrive when stop streamning.
And the EOF will be detected in the next start streaming.
Must ensure clear the left over frame flags before every really start streaming.
Signed-off-by: Albert Wang
Signed-off-by: Libin Yang
Acked-by: Jonathan Corbet
Acked-by: Guennadi Liakhovetski
---
drivers/media/platform/marvell-ccic/mcam-core.c | 30 ---
1 file changed, 26 insertions(+), 4 deletions(-)
diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c
b/drivers/media/platform/marvell-ccic/mcam-core.c
index 7a2855f..a22d17f 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.c
+++ b/drivers/media/platform/marvell-ccic/mcam-core.c
@@ -95,6 +95,9 @@ MODULE_PARM_DESC(buffer_mode,
#define CF_CONFIG_NEEDED 4 /* Must configure hardware */
#define CF_SINGLE_BUFFER 5 /* Running with a single buffer */
#define CF_SG_RESTART 6 /* SG restart needed */
+#define CF_FRAME_SOF0 7 /* Frame 0 started */
+#define CF_FRAME_SOF1 8
+#define CF_FRAME_SOF2 9
#define sensor_call(cam, o, f, args...) \
v4l2_subdev_call(cam->sensor, o, f, ##args)
@@ -261,8 +264,10 @@ static void mcam_reset_buffers(struct mcam_camera *cam)
int i;
cam->next_buf = -1;
- for (i = 0; i < cam->nbufs; i++)
+ for (i = 0; i < cam->nbufs; i++) {
clear_bit(i, &cam->flags);
+ clear_bit(CF_FRAME_SOF0 + i, &cam->flags);
+ }
}
static inline int mcam_needs_config(struct mcam_camera *cam)
@@ -1140,6 +1145,7 @@ static void mcam_vb_wait_finish(struct vb2_queue *vq)
static int mcam_vb_start_streaming(struct vb2_queue *vq, unsigned int count)
{
struct mcam_camera *cam = vb2_get_drv_priv(vq);
+ unsigned int frame;
if (cam->state != S_IDLE) {
INIT_LIST_HEAD(&cam->buffers);
@@ -1157,6 +1163,14 @@ static int mcam_vb_start_streaming(struct vb2_queue *vq,
unsigned int count)
cam->state = S_BUFWAIT;
return 0;
}
+
+ /*
+* Ensure clear the left over frame flags
+* before every really start streaming
+*/
+ for (frame = 0; frame < cam->nbufs; frame++)
+ clear_bit(CF_FRAME_SOF0 + frame, &cam->flags);
+
return mcam_read_setup(cam);
}
@@ -1845,9 +1859,11 @@ int mccic_irq(struct mcam_camera *cam, unsigned int irqs)
* each time.
*/
for (frame = 0; frame < cam->nbufs; frame++)
- if (irqs & (IRQ_EOF0 << frame)) {
+ if (irqs & (IRQ_EOF0 << frame) &&
+ test_bit(CF_FRAME_SOF0 + frame, &cam->flags)) {
mcam_frame_complete(cam, frame);
handled = 1;
+ clear_bit(CF_FRAME_SOF0 + frame, &cam->flags);
if (cam->buffer_mode == B_DMA_sg)
break;
}
@@ -1856,9 +1872,15 @@ int mccic_irq(struct mcam_camera *cam, unsigned int irqs)
* code assumes that we won't get multiple frame interrupts
* at once; may want to rethink that.
*/
- if (irqs & (IRQ_SOF0 | IRQ_SOF1 | IRQ_SOF2)) {
+ for (frame = 0; frame < cam->nbufs; frame++) {
+ if (irqs & (IRQ_SOF0 << frame)) {
+ set_bit(CF_FRAME_SOF0 + frame, &cam->flags);
+ handled = IRQ_HANDLED;
+ }
+ }
+
+ if (handled == IRQ_HANDLED) {
set_bit(CF_DMA_ACTIVE, &cam->flags);
- handled = 1;
if (cam->buffer_mode == B_DMA_sg)
mcam_ctlr_stop(cam);
}
--
1.7.9.5
--
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
[PATCH v2 3/7] marvell-ccic: reset ccic phy when stop streaming for stability
This patch adds the reset ccic phy operation when stop streaming.
Stop streaming without reset ccic phy, the next start streaming
may be unstable.
Also need add CCIC2 definition when PXA688/PXA2128 support dual ccics.
Signed-off-by: Albert Wang
Signed-off-by: Libin Yang
Acked-by: Jonathan Corbet
Acked-by: Guennadi Liakhovetski
---
drivers/media/platform/marvell-ccic/mcam-core.c |6 ++
drivers/media/platform/marvell-ccic/mcam-core.h |2 ++
drivers/media/platform/marvell-ccic/mmp-driver.c | 25 ++
3 files changed, 33 insertions(+)
diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c
b/drivers/media/platform/marvell-ccic/mcam-core.c
index 1e8b3d3..61dd5be 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.c
+++ b/drivers/media/platform/marvell-ccic/mcam-core.c
@@ -1059,6 +1059,12 @@ static int mcam_vb_stop_streaming(struct vb2_queue *vq)
return -EINVAL;
mcam_ctlr_stop_dma(cam);
/*
+* Reset the CCIC PHY after stopping streaming,
+* otherwise, the CCIC may be unstable.
+*/
+ if (cam->ctlr_reset)
+ cam->ctlr_reset(cam);
+ /*
* VB2 reclaims the buffers, so we need to forget
* about them.
*/
diff --git a/drivers/media/platform/marvell-ccic/mcam-core.h
b/drivers/media/platform/marvell-ccic/mcam-core.h
index 6a68aa4..75de293 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.h
+++ b/drivers/media/platform/marvell-ccic/mcam-core.h
@@ -109,6 +109,7 @@ struct mcam_camera {
int mclk_src; /* which clock source the mclk derives from */
int mclk_div; /* Clock Divider Value for MCLK */
+ int ccic_id;
enum v4l2_mbus_type bus_type;
/* MIPI support */
/* The dphy config value, allocated in board file
@@ -130,6 +131,7 @@ struct mcam_camera {
int (*plat_power_up) (struct mcam_camera *cam);
void (*plat_power_down) (struct mcam_camera *cam);
void (*calc_dphy) (struct mcam_camera *cam);
+ void (*ctlr_reset) (struct mcam_camera *cam);
/*
* Everything below here is private to the mcam core and
diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c
b/drivers/media/platform/marvell-ccic/mmp-driver.c
index 3830c44..55fd47b 100644
--- a/drivers/media/platform/marvell-ccic/mmp-driver.c
+++ b/drivers/media/platform/marvell-ccic/mmp-driver.c
@@ -106,6 +106,7 @@ static struct mmp_camera *mmpcam_find_device(struct
platform_device *pdev)
#define CPU_SUBSYS_PMU_BASE0xd4282800
#define REG_CCIC_DCGCR 0x28/* CCIC dyn clock gate ctrl reg */
#define REG_CCIC_CRCR 0x50/* CCIC clk reset ctrl reg */
+#define REG_CCIC2_CRCR 0xf4/* CCIC2 clk reset ctrl reg */
static void mcam_clk_enable(struct mcam_camera *mcam)
{
@@ -193,6 +194,28 @@ static void mmpcam_power_down(struct mcam_camera *mcam)
mcam_clk_disable(mcam);
}
+void mcam_ctlr_reset(struct mcam_camera *mcam)
+{
+ unsigned long val;
+ struct mmp_camera *cam = mcam_to_cam(mcam);
+
+ if (mcam->ccic_id) {
+ /*
+* Using CCIC2
+*/
+ val = ioread32(cam->power_regs + REG_CCIC2_CRCR);
+ iowrite32(val & ~0x2, cam->power_regs + REG_CCIC2_CRCR);
+ iowrite32(val | 0x2, cam->power_regs + REG_CCIC2_CRCR);
+ } else {
+ /*
+* Using CCIC1
+*/
+ val = ioread32(cam->power_regs + REG_CCIC_CRCR);
+ iowrite32(val & ~0x2, cam->power_regs + REG_CCIC_CRCR);
+ iowrite32(val | 0x2, cam->power_regs + REG_CCIC_CRCR);
+ }
+}
+
/*
* calc the dphy register values
* There are three dphy registers being used.
@@ -341,9 +364,11 @@ static int mmpcam_probe(struct platform_device *pdev)
mcam = &cam->mcam;
mcam->plat_power_up = mmpcam_power_up;
mcam->plat_power_down = mmpcam_power_down;
+ mcam->ctlr_reset = mcam_ctlr_reset;
mcam->calc_dphy = mmpcam_calc_dphy;
mcam->dev = &pdev->dev;
mcam->use_smbus = 0;
+ mcam->ccic_id = pdev->id;
mcam->mclk_min = pdata->mclk_min;
mcam->mclk_src = pdata->mclk_src;
mcam->mclk_div = pdata->mclk_div;
--
1.7.9.5
--
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
[PATCH v2 2/7] marvell-ccic: add clock tree support for marvell-ccic driver
This patch adds the clock tree support for marvell-ccic.
Signed-off-by: Libin Yang
Signed-off-by: Albert Wang
---
drivers/media/platform/marvell-ccic/mcam-core.h |6 +++
drivers/media/platform/marvell-ccic/mmp-driver.c | 47 ++
2 files changed, 53 insertions(+)
diff --git a/drivers/media/platform/marvell-ccic/mcam-core.h
b/drivers/media/platform/marvell-ccic/mcam-core.h
index 90162ef..6a68aa4 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.h
+++ b/drivers/media/platform/marvell-ccic/mcam-core.h
@@ -83,6 +83,8 @@ struct mcam_frame_state {
unsigned int delivered;
};
+#define NR_MCAM_CLK 3
+
/*
* A description of one of our devices.
* Locking: controlled by s_mutex. Certain fields, however, require
@@ -118,6 +120,10 @@ struct mcam_camera {
bool mipi_enabled; /* flag whether mipi is enable already */
int lane; /* lane number */
+ /* clock tree support */
+ struct clk *clk[NR_MCAM_CLK];
+
+
/*
* Callbacks from the core to the platform code.
*/
diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c
b/drivers/media/platform/marvell-ccic/mmp-driver.c
index 3b343ce..3830c44 100644
--- a/drivers/media/platform/marvell-ccic/mmp-driver.c
+++ b/drivers/media/platform/marvell-ccic/mmp-driver.c
@@ -35,6 +35,8 @@ MODULE_ALIAS("platform:mmp-camera");
MODULE_AUTHOR("Jonathan Corbet ");
MODULE_LICENSE("GPL");
+static char *mcam_clks[] = {"CCICAXICLK", "CCICFUNCLK", "CCICPHYCLK"};
+
struct mmp_camera {
void *power_regs;
struct platform_device *pdev;
@@ -105,6 +107,26 @@ static struct mmp_camera *mmpcam_find_device(struct
platform_device *pdev)
#define REG_CCIC_DCGCR 0x28/* CCIC dyn clock gate ctrl reg */
#define REG_CCIC_CRCR 0x50/* CCIC clk reset ctrl reg */
+static void mcam_clk_enable(struct mcam_camera *mcam)
+{
+ unsigned int i;
+
+ for (i = 0; i < NR_MCAM_CLK; i++) {
+ if (!IS_ERR(mcam->clk[i]))
+ clk_prepare_enable(mcam->clk[i]);
+ }
+}
+
+static void mcam_clk_disable(struct mcam_camera *mcam)
+{
+ int i;
+
+ for (i = NR_MCAM_CLK - 1; i >= 0; i--) {
+ if (!IS_ERR(mcam->clk[i]))
+ clk_disable_unprepare(mcam->clk[i]);
+ }
+}
+
/*
* Power control.
*/
@@ -135,6 +157,9 @@ static int mmpcam_power_up(struct mcam_camera *mcam)
mdelay(5);
gpio_set_value(pdata->sensor_reset_gpio, 1); /* reset is active low */
mdelay(5);
+
+ mcam_clk_enable(mcam);
+
if (mcam->bus_type == V4L2_MBUS_CSI2) {
cam->mipi_clk = devm_clk_get(mcam->dev, "mipi");
if ((IS_ERR(cam->mipi_clk) && mcam->dphy[2] == 0))
@@ -164,6 +189,8 @@ static void mmpcam_power_down(struct mcam_camera *mcam)
devm_clk_put(mcam->dev, cam->mipi_clk);
cam->mipi_clk = NULL;
}
+
+ mcam_clk_disable(mcam);
}
/*
@@ -274,6 +301,23 @@ static irqreturn_t mmpcam_irq(int irq, void *data)
return IRQ_RETVAL(handled);
}
+static int mcam_init_clk(struct mcam_camera *mcam,
+ struct mmp_camera_platform_data *pdata)
+{
+ unsigned int i;
+
+ for (i = 0; i < NR_MCAM_CLK; i++) {
+ if (mcam_clks[i] != NULL) {
+ mcam->clk[i] = devm_clk_get(mcam->dev, mcam_clks[i]);
+ if (IS_ERR(mcam->clk[i])) {
+ dev_err(mcam->dev, "Could not get clk: %s\n",
+ mcam_clks[i]);
+ return PTR_ERR(mcam->clk[i]);
+ }
+ }
+ }
+ return 0;
+}
static int mmpcam_probe(struct platform_device *pdev)
{
@@ -341,6 +385,9 @@ static int mmpcam_probe(struct platform_device *pdev)
ret = -ENODEV;
goto out_unmap1;
}
+
+ mcam_init_clk(mcam, pdata);
+
/*
* Find the i2c adapter. This assumes, of course, that the
* i2c bus is already up and functioning.
--
1.7.9.5
--
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
[PATCH v2 1/7] marvell-ccic: add MIPI support for marvell-ccic driver
This patch adds the MIPI support for marvell-ccic.
Board driver should determine whether using MIPI or not.
Signed-off-by: Albert Wang
Signed-off-by: Libin Yang
---
drivers/media/platform/marvell-ccic/cafe-driver.c |4 +-
drivers/media/platform/marvell-ccic/mcam-core.c | 78 -
drivers/media/platform/marvell-ccic/mcam-core.h | 37 +-
drivers/media/platform/marvell-ccic/mmp-driver.c | 129 -
include/media/mmp-camera.h| 19 +++
5 files changed, 256 insertions(+), 11 deletions(-)
diff --git a/drivers/media/platform/marvell-ccic/cafe-driver.c
b/drivers/media/platform/marvell-ccic/cafe-driver.c
index d030f9b..68e82fb 100644
--- a/drivers/media/platform/marvell-ccic/cafe-driver.c
+++ b/drivers/media/platform/marvell-ccic/cafe-driver.c
@@ -400,7 +400,7 @@ static void cafe_ctlr_init(struct mcam_camera *mcam)
}
-static void cafe_ctlr_power_up(struct mcam_camera *mcam)
+static int cafe_ctlr_power_up(struct mcam_camera *mcam)
{
/*
* Part one of the sensor dance: turn the global
@@ -415,6 +415,8 @@ static void cafe_ctlr_power_up(struct mcam_camera *mcam)
*/
mcam_reg_write(mcam, REG_GPR, GPR_C1EN|GPR_C0EN); /* pwr up, reset */
mcam_reg_write(mcam, REG_GPR, GPR_C1EN|GPR_C0EN|GPR_C0);
+
+ return 0;
}
static void cafe_ctlr_power_down(struct mcam_camera *mcam)
diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c
b/drivers/media/platform/marvell-ccic/mcam-core.c
index 64ab91e..1e8b3d3 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.c
+++ b/drivers/media/platform/marvell-ccic/mcam-core.c
@@ -19,6 +19,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -254,6 +255,45 @@ static void mcam_ctlr_stop(struct mcam_camera *cam)
mcam_reg_clear_bit(cam, REG_CTRL0, C0_ENABLE);
}
+static void mcam_enable_mipi(struct mcam_camera *mcam)
+{
+ /* Using MIPI mode and enable MIPI */
+ cam_dbg(mcam, "camera: DPHY3=0x%x, DPHY5=0x%x, DPHY6=0x%x\n",
+ mcam->dphy[0], mcam->dphy[1], mcam->dphy[2]);
+ mcam_reg_write(mcam, REG_CSI2_DPHY3, mcam->dphy[0]);
+ mcam_reg_write(mcam, REG_CSI2_DPHY5, mcam->dphy[1]);
+ mcam_reg_write(mcam, REG_CSI2_DPHY6, mcam->dphy[2]);
+
+ if (!mcam->mipi_enabled) {
+ if (mcam->lane > 4 || mcam->lane <= 0) {
+ cam_warn(mcam, "lane number error\n");
+ mcam->lane = 1; /* set the default value */
+ }
+ /*
+* 0x41 actives 1 lane
+* 0x43 actives 2 lanes
+* 0x45 actives 3 lanes (never happen)
+* 0x47 actives 4 lanes
+*/
+ mcam_reg_write(mcam, REG_CSI2_CTRL0,
+ CSI2_C0_MIPI_EN | CSI2_C0_ACT_LANE(mcam->lane));
+ mcam_reg_write(mcam, REG_CLKCTRL,
+ (mcam->mclk_src << 29) | mcam->mclk_div);
+
+ mcam->mipi_enabled = true;
+ }
+}
+
+static void mcam_disable_mipi(struct mcam_camera *mcam)
+{
+ /* Using Parallel mode or disable MIPI */
+ mcam_reg_write(mcam, REG_CSI2_CTRL0, 0x0);
+ mcam_reg_write(mcam, REG_CSI2_DPHY3, 0x0);
+ mcam_reg_write(mcam, REG_CSI2_DPHY5, 0x0);
+ mcam_reg_write(mcam, REG_CSI2_DPHY6, 0x0);
+ mcam->mipi_enabled = false;
+}
+
/* --- */
#ifdef MCAM_MODE_VMALLOC
@@ -657,6 +697,13 @@ static void mcam_ctlr_image(struct mcam_camera *cam)
*/
mcam_reg_write_mask(cam, REG_CTRL0, C0_SIF_HVSYNC,
C0_SIFM_MASK);
+
+ /*
+* This field controls the generation of EOF(DVP only)
+*/
+ if (cam->bus_type != V4L2_MBUS_CSI2)
+ mcam_reg_set_bit(cam, REG_CTRL0,
+ C0_EOF_VSYNC | C0_VEDGE_CTRL);
}
@@ -754,15 +801,21 @@ static void mcam_ctlr_stop_dma(struct mcam_camera *cam)
/*
* Power up and down.
*/
-static void mcam_ctlr_power_up(struct mcam_camera *cam)
+static int mcam_ctlr_power_up(struct mcam_camera *cam)
{
unsigned long flags;
+ int ret;
spin_lock_irqsave(&cam->dev_lock, flags);
- cam->plat_power_up(cam);
+ ret = cam->plat_power_up(cam);
+ if (ret) {
+ spin_unlock_irqrestore(&cam->dev_lock, flags);
+ return ret;
+ }
mcam_reg_clear_bit(cam, REG_CTRL1, C1_PWRDWN);
spin_unlock_irqrestore(&cam->dev_lock, flags);
msleep(5); /* Just to be sure */
+ return 0;
}
static void mcam_ctlr_power_down(struct mcam_camera *cam)
@@ -887,6 +940,17 @@ static int mcam_read_setup(struct mcam_camera *cam)
spin_lock_irqsave(&cam->dev_lock, flags);
clear_bit(CF_DMA_ACTIVE, &cam->fla
RE: [PATCH 2/7] marvell-ccic: add clock tree support for marvell-ccic driver
Hi Jonathan,
Do you mean using IS_ERR() here directly? I think it should be OK. I will
change to IS_ERR() in next version.
Regards,
Libin
>-Original Message-
>From: Jonathan Corbet [mailto:cor...@lwn.net]
>Sent: Saturday, June 22, 2013 1:02 AM
>To: Libin Yang
>Cc: g.liakhovet...@gmx.de; mche...@redhat.com;
>linux-media@vger.kernel.org; albert.v.w...@gmail.com
>Subject: Re: [PATCH 2/7] marvell-ccic: add clock tree support
>for marvell-ccic driver
>
>On Tue, 4 Jun 2013 13:42:44 +0800
>lbyang wrote:
>
>> +static void mcam_clk_enable(struct mcam_camera *mcam) {
>> +unsigned int i;
>> +
>> +for (i = 0; i < NR_MCAM_CLK; i++) {
>> +if (!IS_ERR_OR_NULL(mcam->clk[i]))
>> +clk_prepare_enable(mcam->clk[i]);
>> +}
>> +}
>
>It seems I already acked this patch, and I won't take that
>back. I will point out, though, that IS_ERR_OR_NULL has
>become a sort of lightning rod and that its use is probably
>best avoided.
>
>
>http://lists.infradead.org/pipermail/linux-arm-kernel/2013-Janu
>ary/140543.html
>
>This relates to the use of ERR_PTR with that particular
>pointer value; I still think just using NULL is better, but
>maybe I'm missing something.
>
>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 1/7] marvell-ccic: add MIPI support for marvell-ccic driver
Hi Jonathan,
Sorry for delay reply. Please see the below comments.
Regards,
Libin
>-Original Message-
>From: Jonathan Corbet [mailto:cor...@lwn.net]
>Sent: Saturday, June 22, 2013 1:00 AM
>To: Libin Yang
>Cc: g.liakhovet...@gmx.de; mche...@redhat.com;
>linux-media@vger.kernel.org; albert.v.w...@gmail.com
>Subject: Re: [PATCH 1/7] marvell-ccic: add MIPI support for
>marvell-ccic driver
>
>Better late than never, I hope...in response to Hans's poke,
>I'm going to try to do a quick review. I am allegedly in
>vacation, so this may not be as thorough as we might like...
>
>On Tue, 4 Jun 2013 13:39:40 +0800
>lbyang wrote:
>
>> From: Libin Yang
>>
>> This patch adds the MIPI support for marvell-ccic.
>> Board driver should determine whether using MIPI or not.
>>
>> Signed-off-by: Albert Wang
>> Signed-off-by: Libin Yang
>> ---
>> drivers/media/platform/marvell-ccic/cafe-driver.c |4 +-
>> drivers/media/platform/marvell-ccic/mcam-core.c | 75
>+++-
>> drivers/media/platform/marvell-ccic/mcam-core.h | 32 +-
>> drivers/media/platform/marvell-ccic/mmp-driver.c | 126
>-
>> include/media/mmp-camera.h| 19
>> 5 files changed, 245 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/media/platform/marvell-ccic/cafe-driver.c
>> b/drivers/media/platform/marvell-ccic/cafe-driver.c
>> index d030f9b..68e82fb 100644
>> --- a/drivers/media/platform/marvell-ccic/cafe-driver.c
>> +++ b/drivers/media/platform/marvell-ccic/cafe-driver.c
>> @@ -400,7 +400,7 @@ static void cafe_ctlr_init(struct mcam_camera
>> *mcam) }
>>
>>
>> -static void cafe_ctlr_power_up(struct mcam_camera *mcam)
>> +static int cafe_ctlr_power_up(struct mcam_camera *mcam)
>> {
>> /*
>> * Part one of the sensor dance: turn the global @@
>-415,6 +415,8 @@
>> static void cafe_ctlr_power_up(struct mcam_camera *mcam)
>> */
>> mcam_reg_write(mcam, REG_GPR, GPR_C1EN|GPR_C0EN); /*
>pwr up, reset */
>> mcam_reg_write(mcam, REG_GPR, GPR_C1EN|GPR_C0EN|GPR_C0);
>> +
>> +return 0;
>> }
>
>Curious: why add the return value when it never changes? Do I
>assume that some future patch adds some complexity here? Not
>opposed to this, but it seems like the wrong time.
[Libin] This function will be set to mcam->plat_power_up eventually. The
callback plat_power_up is changed to return int type because of mmp-driver.c
>
>> static void cafe_ctlr_power_down(struct mcam_camera *mcam)
>diff --git
>> a/drivers/media/platform/marvell-ccic/mcam-core.c
>> b/drivers/media/platform/marvell-ccic/mcam-core.c
>> index 64ab91e..bb3de1f 100644
>> --- a/drivers/media/platform/marvell-ccic/mcam-core.c
>> +++ b/drivers/media/platform/marvell-ccic/mcam-core.c
>> @@ -19,6 +19,7 @@
>> #include
>> #include
>> #include
>> +#include
>> #include
>> #include
>> #include
>> @@ -254,6 +255,45 @@ static void mcam_ctlr_stop(struct
>mcam_camera *cam)
>> mcam_reg_clear_bit(cam, REG_CTRL0, C0_ENABLE); }
>>
>> +static int mcam_config_mipi(struct mcam_camera *mcam, bool enable) {
>> +if (enable) {
>> +/* Using MIPI mode and enable MIPI */
>> +cam_dbg(mcam, "camera: DPHY3=0x%x, DPHY5=0x%x,
>DPHY6=0x%x\n",
>> +mcam->dphy[0], mcam->dphy[1], mcam->dphy[2]);
>> +mcam_reg_write(mcam, REG_CSI2_DPHY3, mcam->dphy[0]);
>> +mcam_reg_write(mcam, REG_CSI2_DPHY5, mcam->dphy[1]);
>> +mcam_reg_write(mcam, REG_CSI2_DPHY6, mcam->dphy[2]);
>> +
>> +if (!mcam->mipi_enabled) {
>> +if (mcam->lane > 4 || mcam->lane <= 0) {
>> +cam_warn(mcam, "lane number error\n");
>> +mcam->lane = 1; /* set the
>default value */
>> +}
>> +/*
>> + * 0x41 actives 1 lane
>> + * 0x43 actives 2 lanes
>> + * 0x45 actives 3 lanes (never happen)
>> + * 0x47 actives 4 lanes
>> + */
>> +mcam_reg_write(mcam, REG_CSI2_CTRL0,
>> +CSI2_C0_MIPI_EN |
>CSI2_C0_ACT_LANE(mcam->lane));
>> +mcam_reg_write(mcam, REG_CLKCTRL,
>> +(mcam->mclk_src << 29) |
>mcam->
RE: [REVIEW PATCH V4 01/12] [media] marvell-ccic: add MIPI support for marvell-ccic driver
Hi Guennadi,
>-Original Message-
>From: Guennadi Liakhovetski [mailto:g.liakhovet...@gmx.de]
>Sent: Tuesday, March 12, 2013 6:08 PM
>To: Libin Yang
>Cc: Albert Wang; cor...@lwn.net; Linux Media Mailing List
>Subject: RE: [REVIEW PATCH V4 01/12] [media] marvell-ccic: add MIPI support for
>marvell-ccic driver
>
>Hi Libin
>
>> >
>> >A general comment first: I have no idea about this hardware, so, feel free
>> >to ignore all my hardware-handling related comments. But just from looking
>> >your handling of the pll1 clock does seem a bit fishy to me. You acquire
>> >and release the clock in the generic mcam code, but only use it in the mmp
>> >driver. Is it really needed centrally? Wouldn't it suffice to only acquire
>> >it in mmp? Same goes for your mcam_config_mipi() function - is it really
>> >needed centrally? But as I said, maybe I'm just missing something.
>>
>> [Libin] For the mcam_config_mipi() function, it is used to config mipi
>> in the soc. All boards need to configure it if they are using MIPI based
>> on Marvell CCIC. So I think this function should be in the mcam-core.
>>
>> For the pll1, I think you are right. Actually, it is board based. MMP
>> based boards are using pll1 to calculate the dphy. And I can not
>> guarantee that all boards need pll1. It seems putting pll1 in the
>> mmp-driver is more reasonable. But do you remember, in the previous
>> patch review, you mentioned that it is better to keep the reference to
>> the clock until clean up, because other components may change it. So
>> what I design is: get pll1 and hold it in the open and release it
>> automatically with devm (It may be better to release the pll1 when
>> closing the camera). The problem is in mmp-driver, there is no such
>> point to get the pll1. The open action is in the mcam-core. If I move
>> getting pll1 to the probe function of mmp-driver and putting it in
>> remove, it means camera driver will hold the pll1 all the time. Do you
>> have some suggestions?
>
>Wouldn't it be possible to acquire the clock in mmpcam_power_up() like
>
>struct mmp_camera {
> ...
>+ struct clk *mipi;
>};
>
>static int mmpcam_probe(struct platform_device *pdev)
>{
> ...
>+ cam->mipi = ERR_PTR(-EINVAL);
> ...
>
>-static void mmpcam_power_up(struct mcam_camera *mcam)
>+static int mmpcam_power_up(struct mcam_camera *mcam)
>{
> ...
>+ if (mcam->bus_type == V4L2_MBUS_CSI2 && IS_ERR(cam->mipi)) {
>+ cam->mipi = devm_clk_get(mcam->dev, "mipi");
>+ if (IS_ERR(cam->mipi))
>+ return PTR_ERR(cam->mipi);
>+ }
>
>Yes, it might be good to change the return type of .plat_power_up() to int
>in a separate patch first. And I think a clock name like "mipi" is better
>suitable here, since, as you say, not on all hardware it will be pll1.
[Libin] It is reasonable to acquire the clock in mmpcam_power_up() and release
it in mmpcam_power_down(). So we can get the clock when opening and put the
clock when closing. Using "mipi" is a good suggestion. And I'd like to put
"mipi" clock in the struct mmp_camera, as it is platform related.
>
>> [snip]
>>
>> >
Regards,
Libin
--
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: [REVIEW PATCH V4 01/12] [media] marvell-ccic: add MIPI support for marvell-ccic driver
Hi Guennadi,
Thanks for your careful review. Please help see my comments below.
>-Original Message-
>From: Guennadi Liakhovetski [mailto:g.liakhovet...@gmx.de]
>Sent: Tuesday, March 05, 2013 5:35 AM
>To: Albert Wang
>Cc: cor...@lwn.net; Linux Media Mailing List; Libin Yang
>Subject: Re: [REVIEW PATCH V4 01/12] [media] marvell-ccic: add MIPI support for
>marvell-ccic driver
>
>Hi Albert
>
>A general comment first: I have no idea about this hardware, so, feel free
>to ignore all my hardware-handling related comments. But just from looking
>your handling of the pll1 clock does seem a bit fishy to me. You acquire
>and release the clock in the generic mcam code, but only use it in the mmp
>driver. Is it really needed centrally? Wouldn't it suffice to only acquire
>it in mmp? Same goes for your mcam_config_mipi() function - is it really
>needed centrally? But as I said, maybe I'm just missing something.
[Libin] For the mcam_config_mipi() function, it is used to config mipi in the
soc. All boards need to configure it if they are using MIPI based on Marvell
CCIC. So I think this function should be in the mcam-core.
For the pll1, I think you are right. Actually, it is board based. MMP based
boards are using pll1 to calculate the dphy. And I can not guarantee that all
boards need pll1. It seems putting pll1 in the mmp-driver is more reasonable.
But do you remember, in the previous patch review, you mentioned that it is
better to keep the reference to the clock until clean up, because other
components may change it. So what I design is: get pll1 and hold it in the open
and release it automatically with devm (It may be better to release the pll1
when closing the camera). The problem is in mmp-driver, there is no such point
to get the pll1. The open action is in the mcam-core. If I move getting pll1 to
the probe function of mmp-driver and putting it in remove, it means camera
driver will hold the pll1 all the time. Do you have some suggestions?
[snip]
>
>> +if (ret < 0)
>> +return ret;
>> mcam_ctlr_irq_enable(cam);
>> cam->state = S_STREAMING;
>> if (!test_bit(CF_SG_RESTART, &cam->flags))
>> @@ -1551,6 +1602,16 @@ static int mcam_v4l_open(struct file *filp)
>> mcam_set_config_needed(cam, 1);
>> }
>> (cam->users)++;
>> +if (cam->bus_type == V4L2_MBUS_CSI2) {
>> +cam->pll1 = devm_clk_get(cam->dev, "pll1");
>> +if (IS_ERR_OR_NULL(cam->pll1) && cam->dphy[2] == 0) {
>
>So, is CSI2 mode only supported with enabled CONFIG_HAVE_CLK? It looks a
>bit susppicious, but, I think, this might be valid here - you really need
>a clock, from which you can read a valid rate to use CSI2, right?
>Otherwise you cannot configure dphy.
[Libin] If dphy[2] is initialized, it is OK pll1 is not used. Please see the
related comments below in the function mmpcam_calc_dphy()
>
>> + */
>> +void mmpcam_calc_dphy(struct mcam_camera *mcam)
>> +{
>> +struct mmp_camera *cam = mcam_to_cam(mcam);
>> +struct mmp_camera_platform_data *pdata = cam->pdev->dev.platform_data;
>> +struct device *dev = &cam->pdev->dev;
[snip]
>> +}
>> +
>> +/*
>> + * pll1 will never be changed, it is a fixed value
>> + */
>> +
>> +if (IS_ERR_OR_NULL(mcam->pll1))
>> +return;
>
>All this function does is calculate dphy[] array values, right? And these
>values are only used if CSI2 is activated. And CSI2 can only be activated
>if an open() has been successful. And you only succeed a CSI2-mode open()
>if a clock can be acquired. So, the above check is redundant?
[Libin] pll1 is used to calculate dphy[2]. If dphy[2] is initialized then pll1
being NULL is OK. Please see the open function. If pll1 is not NULL, then we
will calculate the dphy[2] based on pll1.
>
>> +
>> +/* get the escape clk, this is hard coded */
>> +tx_clk_esc = (clk_get_rate(mcam->pll1) / 100) / 12;
>> +
>> +/*
>> + * dphy[2] - CSI2_DPHY6:
>> + * bit 0 ~ bit 7: CK Term Enable
>> + * Time for the Clock Lane receiver to enable the HS line
Regards,
Libin
--
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 V3 06/15] [media] marvell-ccic: add new formats support for marvell-ccic driver
Hi Guennadi,
Below is the update for widthy, widthuv and imgsz_w setting.
>-Original Message-
>From: Guennadi Liakhovetski [mailto:g.liakhovet...@gmx.de]
>Sent: Wednesday, January 02, 2013 12:56 AM
>To: Albert Wang
>Cc: cor...@lwn.net; linux-media@vger.kernel.org; Libin Yang
>Subject: Re: [PATCH V3 06/15] [media] marvell-ccic: add new formats support for
>marvell-ccic driver
>
>On Sat, 15 Dec 2012, Albert Wang wrote:
>
>> From: Libin Yang
>>
>> This patch adds the new formats support for marvell-ccic.
>>
>> Signed-off-by: Albert Wang
>> Signed-off-by: Libin Yang
>> ---
>> drivers/media/platform/marvell-ccic/mcam-core.c | 175
>> ++-
>> drivers/media/platform/marvell-ccic/mcam-core.h |6 +
>> 2 files changed, 149 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c
>b/drivers/media/platform/marvell-ccic/mcam-core.c
>> index 3cc1d0c..a679917 100755
>> --- a/drivers/media/platform/marvell-ccic/mcam-core.c
>> +++ b/drivers/media/platform/marvell-ccic/mcam-core.c
>
>[snip]
>
>> @@ -658,49 +708,85 @@ static inline void mcam_sg_restart(struct mcam_camera
>> *cam)
>> */
>> static void mcam_ctlr_image(struct mcam_camera *cam)
>> {
>> -int imgsz;
>> struct v4l2_pix_format *fmt = &cam->pix_format;
>> +u32 widthy = 0, widthuv = 0, imgsz_h, imgsz_w;
>> +
>> +cam_dbg(cam, "camera: bytesperline = %d; height = %d\n",
>> +fmt->bytesperline, fmt->sizeimage / fmt->bytesperline);
>> +imgsz_h = (fmt->height << IMGSZ_V_SHIFT) & IMGSZ_V_MASK;
>> +imgsz_w = fmt->bytesperline & IMGSZ_H_MASK;
>> +
>> +switch (fmt->pixelformat) {
>> +case V4L2_PIX_FMT_YUYV:
>> +case V4L2_PIX_FMT_UYVY:
>> +widthy = fmt->width * 2;
>> +widthuv = 0;
>> +break;
>> +case V4L2_PIX_FMT_JPEG:
>> +imgsz_h = (fmt->sizeimage / fmt->bytesperline) << IMGSZ_V_SHIFT;
>> +widthy = fmt->bytesperline;
>> +widthuv = 0;
>> +break;
>> +case V4L2_PIX_FMT_YUV422P:
>> +case V4L2_PIX_FMT_YUV420:
>> +case V4L2_PIX_FMT_YVU420:
>> +imgsz_w = (fmt->bytesperline * 4 / 3) & IMGSZ_H_MASK;
>> +widthy = fmt->width;
>> +widthuv = fmt->width / 2;
>
>I might be wrong, but the above doesn't look right to me. Firstly, YUV422P
>is a 4:2:2 format, whereas YUV420 and YVU420 are 4:2:0 formats, so, I
>would expect calculations for them to differ. Besides, bytesperline * 4 /
>3 doesn't look right for any of them. If this is what I think - total
>number of bytes per line, i.e., sizeimage / height, than shouldn't YAU422P
>have
>+ imgsz_w = fmt->bytesperline & IMGSZ_H_MASK;
>and the other two
>+ imgsz_w = (fmt->bytesperline * 3 / 2) & IMGSZ_H_MASK;
>? But maybe I'm wrong, please, double-check and confirm.
>
First of all, the setting bytesperline in this file is wrong. It should be
like the setting in the mcam-core-soc.c in the later patch. It's my fault
missing modifying the bytesperline when adding the new formats.
Besides the bytesperline in mcam-core-soc.c is a little wrong.
I will update it in the new version of patch.
For imgsz_w setting, this is for the CCIC input data format, which is from
sensor, while 'switch (fmt->pixelformat)' is CCIC output data format.
It seems a little confusing using fmt->pixelformat here. Using
mcam_formats->mbus_code seems more reasonable. Anyway,
each fmt->pixelformat must have one mcam_formats->mbus_code
correspondingly.
Although, our spec says it supports YUV420 input. Our HW team told me
we only use YUV422 and the length is width * 2. So it seems some
mbus_code is wrong set here.
It seems in this case such format will be never used as we can see
ov7670 does not support yuv420.
Regards,
Libin
--
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 V3 03/15] [media] marvell-ccic: add clock tree support for marvell-ccic driver
Hi Guennadi,
Please see my comments below.
>-Original Message-
>From: Guennadi Liakhovetski [mailto:g.liakhovet...@gmx.de]
>Sent: Friday, January 04, 2013 6:25 PM
>To: Libin Yang
>Cc: Albert Wang; cor...@lwn.net; linux-media@vger.kernel.org
>Subject: RE: [PATCH V3 03/15] [media] marvell-ccic: add clock tree support for
>marvell-ccic driver
>
>Hi Libin
>
>On Thu, 3 Jan 2013, Libin Yang wrote:
>
>> Hi Guennadi,
>>
>> Thanks for your review. Please see my comments below.
>>
[snip]
>> >> /*
>> >> @@ -202,7 +223,7 @@ void mmpcam_calc_dphy(struct mcam_camera *mcam)
>> >>* pll1 will never be changed, it is a fixed value
>> >>*/
>> >>
>> >> - if (IS_ERR(mcam->pll1))
>> >> + if (IS_ERR_OR_NULL(mcam->pll1))
>> >
>> >Why are you changing this? If this really were needed, you should do this
>> >already in the previous patch, where you add these lines. But I don't
>> >think this is a good idea, don't think Russell would like this :-) NULL is
>> >a valid clock. Only a negative error is a failure. In fact, if you like,
>> >you could initialise .pll1 to ERR_PTR(-EINVAL) in your previous patch in
>> >mmpcam_probe().
>>
>> In the below code, we will use platform related clk_get_rate() to get the
>> rate.
>> In the function we do not judge the clk is NULL or not. If we do not judge
>> here,
>> we need judge for NULL in the later, otherwise, error may happen. Or do you
>> think it is better that we should judge the pointer in the function
>> clk_get_rate()?
>
>I think, there is a problem here. Firstly, if you really want to check for
>"clock API not supported" or a similar type of condition by checking
>get_clk() return value for NULL, you should do this immediately in the
>patch, where you add this code: in "[PATCH V3 02/15] [media] marvell-ccic:
>add MIPI support for marvell-ccic driver." Secondly, it's probably ok to
>check this to say - no clock, co reason to try to use it, in which case
>you skip calculating your ->dphy[2] value, and it remains == 0,
>presumably, is this what you want to have? But, I think, there's a bigger
>problem in your patch #02/15: you don't check for mcam->dphy != NULL. So,
>I think, this has to be fixed in that patch, not here.
Your understanding is right. We will try to use the default value if the pll1
is not available. So we just return if pll1 is error or NULL. And mostly the
default value should work. And this reminds me that there is a little issue:
we should not return fail in the mcam_v4l_open() function when failing
to get the pll1 clk because we can also use the default values.
What I plan to code in the next version is:
1. Remove the judgement of (IS_ERR(cam->pll1)) in the open function when
get the clk.
2. Still use IS_ERR_OR_NULL(mcam->pll1), so that we can use the default
vaule if pll1 is not available.
What do you think of it?
mcam->dphy != NULL may be a critical issue. Thanks for digging out this bug.
We will fix it in the next version.
>
>[snip]
>
>> >> +{
>> >> + unsigned int i;
>> >> +
>> >> + if (NR_MCAM_CLK < pdata->clk_num) {
>> >> + dev_err(mcam->dev, "Too many mcam clocks defined\n");
>> >> + mcam->clk_num = 0;
>> >> + return;
>> >> + }
>> >> +
>> >> + if (init) {
>> >> + for (i = 0; i < pdata->clk_num; i++) {
>> >> + if (pdata->clk_name[i] != NULL) {
>> >> + mcam->clk[i] = devm_clk_get(mcam->dev,
>> >> + pdata->clk_name[i]);
>> >
>> >Sorry, no. Passing clock names in platform data doesn't look right to me.
>> >Clock names are a property of the consumer device, not of clock supplier.
>> >Also, your platform tells you to get clk_num clocks, you fail to get one
>> >of them, you don't continue trying the rest and just return with no error.
>> >This seems strange, usually a failure to get clocks, that the platform
>> >tells you to get, is fatal.
>>
>> I agree that after failing to get the clk, we should return error
>> instead of just returning.
>>
>> For the clock names, the clock names are different on different platforms.
>> So we need platform data passing the clock names. Do you have any
>> suggestions?
>
>I think you should use the same names on all platforms. As I said, those
>are names of _consumer_ clocks, not of supplier. And the consumer on
RE: [PATCH V3 06/15] [media] marvell-ccic: add new formats support for marvell-ccic driver
Hi Guennadi,
Please see my comments below.
>-Original Message-
>From: Guennadi Liakhovetski [mailto:g.liakhovet...@gmx.de]
>Sent: Wednesday, January 02, 2013 12:56 AM
>To: Albert Wang
>Cc: cor...@lwn.net; linux-media@vger.kernel.org; Libin Yang
>Subject: Re: [PATCH V3 06/15] [media] marvell-ccic: add new formats support for
>marvell-ccic driver
>
>On Sat, 15 Dec 2012, Albert Wang wrote:
>
>> From: Libin Yang
>>
>> This patch adds the new formats support for marvell-ccic.
>>
>> Signed-off-by: Albert Wang
>> Signed-off-by: Libin Yang
>> ---
>> drivers/media/platform/marvell-ccic/mcam-core.c | 175
>> ++-
>> drivers/media/platform/marvell-ccic/mcam-core.h |6 +
>> 2 files changed, 149 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c
>b/drivers/media/platform/marvell-ccic/mcam-core.c
>> index 3cc1d0c..a679917 100755
>> --- a/drivers/media/platform/marvell-ccic/mcam-core.c
>> +++ b/drivers/media/platform/marvell-ccic/mcam-core.c
>
>[snip]
>
>> @@ -658,49 +708,85 @@ static inline void mcam_sg_restart(struct mcam_camera
>> *cam)
>> */
>> static void mcam_ctlr_image(struct mcam_camera *cam)
>> {
>> -int imgsz;
>> struct v4l2_pix_format *fmt = &cam->pix_format;
>> +u32 widthy = 0, widthuv = 0, imgsz_h, imgsz_w;
>> +
>> +cam_dbg(cam, "camera: bytesperline = %d; height = %d\n",
>> +fmt->bytesperline, fmt->sizeimage / fmt->bytesperline);
>> +imgsz_h = (fmt->height << IMGSZ_V_SHIFT) & IMGSZ_V_MASK;
>> +imgsz_w = fmt->bytesperline & IMGSZ_H_MASK;
>> +
>> +switch (fmt->pixelformat) {
>> +case V4L2_PIX_FMT_YUYV:
>> +case V4L2_PIX_FMT_UYVY:
>> +widthy = fmt->width * 2;
>> +widthuv = 0;
>> +break;
>> +case V4L2_PIX_FMT_JPEG:
>> +imgsz_h = (fmt->sizeimage / fmt->bytesperline) << IMGSZ_V_SHIFT;
>> +widthy = fmt->bytesperline;
>> +widthuv = 0;
>> +break;
>> +case V4L2_PIX_FMT_YUV422P:
>> +case V4L2_PIX_FMT_YUV420:
>> +case V4L2_PIX_FMT_YVU420:
>> +imgsz_w = (fmt->bytesperline * 4 / 3) & IMGSZ_H_MASK;
>> +widthy = fmt->width;
>> +widthuv = fmt->width / 2;
>
>I might be wrong, but the above doesn't look right to me. Firstly, YUV422P
>is a 4:2:2 format, whereas YUV420 and YVU420 are 4:2:0 formats, so, I
>would expect calculations for them to differ. Besides, bytesperline * 4 /
>3 doesn't look right for any of them. If this is what I think - total
>number of bytes per line, i.e., sizeimage / height, than shouldn't YAU422P
>have
>+ imgsz_w = fmt->bytesperline & IMGSZ_H_MASK;
>and the other two
>+ imgsz_w = (fmt->bytesperline * 3 / 2) & IMGSZ_H_MASK;
>? But maybe I'm wrong, please, double-check and confirm.
>
Basically, I agree with you. We are checking with our HW team how the value is
calculated out. And we will update here as soon as we get the answer.
>> +break;
>> +default:
>> +widthy = fmt->bytesperline;
>> +widthuv = 0;
>> +}
>> +
>> +mcam_reg_write_mask(cam, REG_IMGPITCH, widthuv << 16 | widthy,
>> +IMGP_YP_MASK | IMGP_UVP_MASK);
>> +mcam_reg_write(cam, REG_IMGSIZE, imgsz_h | imgsz_w);
>> +mcam_reg_write(cam, REG_IMGOFFSET, 0x0);
>>
>> -imgsz = ((fmt->height << IMGSZ_V_SHIFT) & IMGSZ_V_MASK) |
>> -(fmt->bytesperline & IMGSZ_H_MASK);
>> -mcam_reg_write(cam, REG_IMGSIZE, imgsz);
>> -mcam_reg_write(cam, REG_IMGOFFSET, 0);
>> -/* YPITCH just drops the last two bits */
>> -mcam_reg_write_mask(cam, REG_IMGPITCH, fmt->bytesperline,
>> -IMGP_YP_MASK);
>> /*
>> * Tell the controller about the image format we are using.
>> */
>> -switch (cam->pix_format.pixelformat) {
>> +switch (fmt->pixelformat) {
>> +case V4L2_PIX_FMT_YUV422P:
>> +mcam_reg_write_mask(cam, REG_CTRL0,
>> +C0_DF_YUV | C0_YUV_PLANAR | C0_YUVE_YVYU,
>C0_DF_MASK);
>> +break;
>> +case V4L2_PIX_FMT_YUV420:
>> +case V4L2_PIX_FMT_YVU420:
>> +mcam_reg_write_mask(cam, REG_CTRL0,
>> +C0_DF_YUV | C0_YUV_420PL | C0_YUVE_YVYU, C0_DF_MASK);
>&g
RE: [PATCH V3 03/15] [media] marvell-ccic: add clock tree support for marvell-ccic driver
Hi Guennadi,
Thanks for your review. Please see my comments below.
>-Original Message-
>From: Guennadi Liakhovetski [mailto:g.liakhovet...@gmx.de]
>Sent: Wednesday, January 02, 2013 12:06 AM
>To: Albert Wang
>Cc: cor...@lwn.net; linux-media@vger.kernel.org; Libin Yang
>Subject: Re: [PATCH V3 03/15] [media] marvell-ccic: add clock tree support for
>marvell-ccic driver
>
>On Sat, 15 Dec 2012, Albert Wang wrote:
>
>> From: Libin Yang
>>
>> This patch adds the clock tree support for marvell-ccic.
>>
>> Each board may require different clk enabling sequence.
>> Developer need add the clk_name in correct sequence in board driver
>> to use this feature.
>>
>> Signed-off-by: Libin Yang
>> Signed-off-by: Albert Wang
>> ---
>> drivers/media/platform/marvell-ccic/mcam-core.h |4 ++
>> drivers/media/platform/marvell-ccic/mmp-driver.c | 57
>> +-
>> include/media/mmp-camera.h |5 ++
>> 3 files changed, 65 insertions(+), 1 deletion(-)
>>
[snip]
>> diff --git a/drivers/media/platform/marvell-ccic/mmp-driver.c
>b/drivers/media/platform/marvell-ccic/mmp-driver.c
>> index 603fa0a..2c4dce3 100755
>> --- a/drivers/media/platform/marvell-ccic/mmp-driver.c
>> +++ b/drivers/media/platform/marvell-ccic/mmp-driver.c
[snip]
>> +
>> +mcam_clk_set(mcam, 0);
>> }
>>
>> /*
>> @@ -202,7 +223,7 @@ void mmpcam_calc_dphy(struct mcam_camera *mcam)
>> * pll1 will never be changed, it is a fixed value
>> */
>>
>> -if (IS_ERR(mcam->pll1))
>> +if (IS_ERR_OR_NULL(mcam->pll1))
>
>Why are you changing this? If this really were needed, you should do this
>already in the previous patch, where you add these lines. But I don't
>think this is a good idea, don't think Russell would like this :-) NULL is
>a valid clock. Only a negative error is a failure. In fact, if you like,
>you could initialise .pll1 to ERR_PTR(-EINVAL) in your previous patch in
>mmpcam_probe().
In the below code, we will use platform related clk_get_rate() to get the rate.
In the function we do not judge the clk is NULL or not. If we do not judge
here,
we need judge for NULL in the later, otherwise, error may happen. Or do you
think it is better that we should judge the pointer in the function
clk_get_rate()?
>
>> return;
>>
>> tx_clk_esc = clk_get_rate(mcam->pll1) / 100 / 12;
>> @@ -229,6 +250,35 @@ static irqreturn_t mmpcam_irq(int irq, void *data)
>> return IRQ_RETVAL(handled);
>> }
>>
>> +static void mcam_init_clk(struct mcam_camera *mcam,
>> +struct mmp_camera_platform_data *pdata, int init)
>
>I don't think this "int init" makes sense. Please, remove the parameter
>and you actually don't need the de-initialisation, no reason to set
>num_clk = 0 before freeing memory.
Yes, the init parameter is not good here, which make confusion.
The driver de-initiatives the clks because
I want to make sure the driver will never enable the clks after
de-initialization.
After second consideration based on your suggestion, I will remove
de-initialization, because this scenario will never happen.
>
>> +{
>> +unsigned int i;
>> +
>> +if (NR_MCAM_CLK < pdata->clk_num) {
>> +dev_err(mcam->dev, "Too many mcam clocks defined\n");
>> +mcam->clk_num = 0;
>> +return;
>> +}
>> +
>> +if (init) {
>> +for (i = 0; i < pdata->clk_num; i++) {
>> +if (pdata->clk_name[i] != NULL) {
>> +mcam->clk[i] = devm_clk_get(mcam->dev,
>> +pdata->clk_name[i]);
>
>Sorry, no. Passing clock names in platform data doesn't look right to me.
>Clock names are a property of the consumer device, not of clock supplier.
>Also, your platform tells you to get clk_num clocks, you fail to get one
>of them, you don't continue trying the rest and just return with no error.
>This seems strange, usually a failure to get clocks, that the platform
>tells you to get, is fatal.
I agree that after failing to get the clk, we should return error
instead of just returning.
For the clock names, the clock names are different on different platforms.
So we need platform data passing the clock names. Do you have any suggestions?
>
>> +if (IS_ERR(mcam->clk[i])) {
>> +dev_err(mcam->dev,
>> +"Could not
RE: [PATCH 06/15] [media] marvell-ccic: add new formats support for marvell-ccic driver
Hi Guennadi,
>-Original Message-
>From: Guennadi Liakhovetski [mailto:g.liakhovet...@gmx.de]
>Sent: Tuesday, December 04, 2012 2:57 PM
>To: Libin Yang
>Cc: cor...@lwn.net; linux-media@vger.kernel.org
>Subject: RE: [PATCH 06/15] [media] marvell-ccic: add new formats support for
>marvell-ccic
>driver
>
>Hi Libin
>
>On Mon, 3 Dec 2012, Libin Yang wrote:
>
>> Hi Guennadi,
>>
>> When I'm refining the patch based on your comments, I met an issue.
>> Could you please help me?
>>
>> [snip]
>>
>>
>> >>> -
>> >>> /*
>> >>> * Configure the controller for operation; caller holds the
>> >>> * device mutex.
>> >>> @@ -979,11 +1070,32 @@ static void mcam_vb_buf_queue(struct vb2_buffer
>> >>> *vb) {
>> >>> struct mcam_vb_buffer *mvb = vb_to_mvb(vb);
>> >>> struct mcam_camera *cam = vb2_get_drv_priv(vb->vb2_queue);
>> >>> +struct v4l2_pix_format *fmt = &cam->pix_format;
>> >>> unsigned long flags;
>> >>> int start;
>> >>> +dma_addr_t dma_handle;
>> >>> +u32 base_size = fmt->width * fmt->height;
>> >>
>> >>Shouldn't you be just using bytesperline? Is stride != width * height
>> >>supported?
>> >>
>> >We will update it.
>>
>> [Libin] What I understand is width is the pixel number perline, so
>> bytesperline = width * BytePerPixel. This means bytesperline should
>> include Y data and UV data.
>>
>> For example, for yuv420 legacy 8-bit, it transfers like below:
>> U Y Y U Y Y U Y Y U Y Y ..
>> V Y Y V Y Y V Y Y V Y Y ..
>>
>> As each Y is one byte, so all Y data length is nuber_Y_per_line *
>> height. While the nuber_Y_per_line is the pixel_per_line, which is
>> fmt->width.
>>
>> So for the planner, the first block is saving the Y data, whose length
>> is nuber_Y_per_line * height, which equals to fmt->width * height.
>>
>> Do I understand correctly?
>>
>> The base_size here means the size of Y data, so it should be fmt->width
>> * fmt->height.
>
>Right, in this case you're right. Sorry for a misleading comment. Just a
>suggestion, if you prefer, you can keep the name, but maybe a name like
>pixel_count or pixel_num or pixel_per_frame would be clearer for that
>variable? But keeping the name is also ok with me.
>
[Libin] OK, I see. Thanks for your suggestion. The name really seems a little
misleading. I will use a more reasonable name in the coming patch.
>> >>>
>> >>> spin_lock_irqsave(&cam->dev_lock, flags);
>> >>> +dma_handle = vb2_dma_contig_plane_dma_addr(vb, 0);
>
>Thanks
>Guennadi
>---
>Guennadi Liakhovetski, Ph.D.
>Freelance Open-Source Software Developer
>http://www.open-technology.de/
Regards,
Libin
--
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 06/15] [media] marvell-ccic: add new formats support for marvell-ccic driver
Hi Guennadi,
When I'm refining the patch based on your comments, I met an issue. Could you
please help me?
[snip]
>>> -
>>> /*
>>> * Configure the controller for operation; caller holds the
>>> * device mutex.
>>> @@ -979,11 +1070,32 @@ static void mcam_vb_buf_queue(struct vb2_buffer
>>> *vb) {
>>> struct mcam_vb_buffer *mvb = vb_to_mvb(vb);
>>> struct mcam_camera *cam = vb2_get_drv_priv(vb->vb2_queue);
>>> + struct v4l2_pix_format *fmt = &cam->pix_format;
>>> unsigned long flags;
>>> int start;
>>> + dma_addr_t dma_handle;
>>> + u32 base_size = fmt->width * fmt->height;
>>
>>Shouldn't you be just using bytesperline? Is stride != width * height
>>supported?
>>
>We will update it.
[Libin] What I understand is width is the pixel number perline, so bytesperline
= width * BytePerPixel. This means bytesperline should include Y data and UV
data.
For example, for yuv420 legacy 8-bit, it transfers like below:
U Y Y U Y Y U Y Y U Y Y ..
V Y Y V Y Y V Y Y V Y Y ..
As each Y is one byte, so all Y data length is nuber_Y_per_line * height. While
the nuber_Y_per_line is the pixel_per_line, which is fmt->width.
So for the planner, the first block is saving the Y data, whose length is
nuber_Y_per_line * height, which equals to fmt->width * height.
Do I understand correctly?
The base_size here means the size of Y data, so it should be fmt->width *
fmt->height.
>>>
>>> spin_lock_irqsave(&cam->dev_lock, flags);
>>> + dma_handle = vb2_dma_contig_plane_dma_addr(vb, 0);
Best Regards,
Libin
--
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 02/15] [media] marvell-ccic: add MIPI support for marvell-ccic driver
Hi Guennadi, [snip] >> [Libin] Yes, you are right. We should consider the driver may be reused. >> I didn't realize it. Another question is: If we use devm_clk_get(), what >> I understand, the clk will be put when the device is being released. It >> means the driver will hold the clk all the time the driver is in the >> kernel. What do you think if we get the clk when opening the camera, and >> put it in the close? > >Sure, that's fine too. OK, I see. Thanks. Regards, Libin -- 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 02/15] [media] marvell-ccic: add MIPI support for marvell-ccic driver
Hi Guennadi,
>-Original Message-
>From: Guennadi Liakhovetski [mailto:g.liakhovet...@gmx.de]
>Sent: Wednesday, November 28, 2012 3:14 PM
>To: Libin Yang
>Cc: Albert Wang; cor...@lwn.net; linux-media@vger.kernel.org
>Subject: RE: [PATCH 02/15] [media] marvell-ccic: add MIPI support for
>marvell-ccic driver
>
>On Tue, 27 Nov 2012, Libin Yang wrote:
>
>> Hello Guennadi,
>>
>> Please see my comments below.
>>
>> Best Regards,
>> Libin
>>
>> >-Original Message-
>> >From: Albert Wang
>> >Sent: Tuesday, November 27, 2012 7:21 PM
>> >To: Guennadi Liakhovetski
>> >Cc: cor...@lwn.net; linux-media@vger.kernel.org; Libin Yang
>> >Subject: RE: [PATCH 02/15] [media] marvell-ccic: add MIPI support for
>> >marvell-ccic
>driver
>> >
>> >Hi, Guennadi
>> >
>> >We will update the patch by following your good suggestion! :)
>> >
>>
>> [snip]
>>
>> >>> +pll1 = clk_get(dev, "pll1");
>> >>> +if (IS_ERR(pll1)) {
>> >>> +dev_err(dev, "Could not get pll1 clock\n");
>> >>> +return;
>> >>> +}
>> >>> +
>> >>> +tx_clk_esc = clk_get_rate(pll1) / 100 / 12;
>> >>> +clk_put(pll1);
>> >>
>> >>Once you release your clock per "clk_put()" its rate can be changed by
>> >>some other user,
>> >>so, your tx_clk_esc becomes useless. Better keep the reference to the
>> >>clock until clean
>up.
>> >>Maybe you can also use
>> >>devm_clk_get() to simplify the clean up.
>> >>
>> >That's a good suggestion.
>> >
>> [Libin] In our code design, the pll1 will never be changed after the system
>> boots up.
>Camera and other components can only get the clk without modifying it.
>
>This doesn't matter. We have a standard API and we have to abide to its
>rules. Your driver can be reused or its code can be copied by others. I
>don't think it should be too difficult to just issue devm_clk_get() once
>and then just forget about it.
[Libin] Yes, you are right. We should consider the driver may be reused. I
didn't realize it. Another question is: If we use devm_clk_get(), what I
understand, the clk will be put when the device is being released. It means the
driver will hold the clk all the time the driver is in the kernel. What do you
think if we get the clk when opening the camera, and put it in the close?
>
>Thanks
>Guennadi
>---
>Guennadi Liakhovetski, Ph.D.
>Freelance Open-Source Software Developer
>http://www.open-technology.de/
Best Regard,
Libin
--
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 02/15] [media] marvell-ccic: add MIPI support for marvell-ccic driver
Hello Guennadi,
Please see my comments below.
Best Regards,
Libin
>-Original Message-
>From: Albert Wang
>Sent: Tuesday, November 27, 2012 7:21 PM
>To: Guennadi Liakhovetski
>Cc: cor...@lwn.net; linux-media@vger.kernel.org; Libin Yang
>Subject: RE: [PATCH 02/15] [media] marvell-ccic: add MIPI support for
>marvell-ccic driver
>
>Hi, Guennadi
>
>We will update the patch by following your good suggestion! :)
>
[snip]
>>> + pll1 = clk_get(dev, "pll1");
>>> + if (IS_ERR(pll1)) {
>>> + dev_err(dev, "Could not get pll1 clock\n");
>>> + return;
>>> + }
>>> +
>>> + tx_clk_esc = clk_get_rate(pll1) / 100 / 12;
>>> + clk_put(pll1);
>>
>>Once you release your clock per "clk_put()" its rate can be changed by some
>>other user,
>>so, your tx_clk_esc becomes useless. Better keep the reference to the clock
>>until clean up.
>>Maybe you can also use
>>devm_clk_get() to simplify the clean up.
>>
>That's a good suggestion.
>
[Libin] In our code design, the pll1 will never be changed after the system
boots up. Camera and other components can only get the clk without modifying it.
--
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 12/15] [media] marvell-ccic: add soc_camera support in mmp driver
Hello Guennadi, Please see my comments below. Regards, Libin >-Original Message- >From: Albert Wang >Sent: Wednesday, November 28, 2012 12:06 AM >To: Guennadi Liakhovetski >Cc: cor...@lwn.net; linux-media@vger.kernel.org; Libin Yang >Subject: RE: [PATCH 12/15] [media] marvell-ccic: add soc_camera support in mmp >driver > >Hi, Guennadi > > >>-Original Message- >>From: Guennadi Liakhovetski [mailto:g.liakhovet...@gmx.de] >>Sent: Tuesday, 27 November, 2012 23:54 >>To: Albert Wang >>Cc: cor...@lwn.net; linux-media@vger.kernel.org; Libin Yang >>Subject: Re: [PATCH 12/15] [media] marvell-ccic: add soc_camera support in mmp >>driver >> [snip] >>> >>> mcam = &cam->mcam; >>> + spin_lock_init(&mcam->dev_lock); >>> mcam->plat_power_up = mmpcam_power_up; >>> mcam->plat_power_down = mmpcam_power_down; >>> mcam->ctlr_reset = mcam_ctlr_reset; >>> mcam->calc_dphy = mmpcam_calc_dphy; >>> mcam->dev = &pdev->dev; >>> mcam->use_smbus = 0; >>> + mcam->card_name = pdata->name; >>> + mcam->mclk_min = pdata->mclk_min; >>> + mcam->mclk_src = pdata->mclk_src; >>> + mcam->mclk_div = pdata->mclk_div; >> >>Actually you don't really have to copy everything from platform data to your >>private >>driver object during probing. You can access your platform data also at >>run-time. So, >>maybe you can survive without adding these >>.mclk_* struct members? >> >Yes, make sense. :) [Libin] We add such members because we need use these variables in the file mcam-core-soc.c. In the mcam-core-soc.c, the pdata is invisible. I think we can split the probe function and copy them in the file mcam-core-soc.c as you suggested. [snip] >>> + int chip_id; >>> /* >>> * MIPI support >>> */ >>> -- >>> 1.7.9.5 >>> >> >>Thanks >>Guennadi >>--- >>Guennadi Liakhovetski, Ph.D. >>Freelance Open-Source Software Developer >>http://www.open-technology.de/ -- 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 03/15] [media] marvell-ccic: add clock tree support for marvell-ccic driver
Hello Guennadi,
Thanks for your suggestion, please see my comments below.
Best Regards,
Libin
>>-Original Message-
>>From: Guennadi Liakhovetski [mailto:g.liakhovet...@gmx.de]
>>Sent: Tuesday, 27 November, 2012 18:50
>>To: Albert Wang
>>Cc: cor...@lwn.net; linux-media@vger.kernel.org; Libin Yang
>>Subject: Re: [PATCH 03/15] [media] marvell-ccic: add clock tree support for
>>marvell-ccic
>>driver
>>
>>> + mcam->clk_num = pdata->clk_num;
>>> + } else {
>>> + for (i = 0; i < pdata->clk_num; i++) {
>>> + if (mcam->clk[i]) {
>>> + clk_put(mcam->clk[i]);
>>> + mcam->clk[i] = NULL;
>>> + }
>>> + }
>>> + mcam->clk_num = 0;
>>> + }
>>> +}
>>
>>Don't think I like this. IIUC, your driver should only try to use clocks,
>>that it knows about,
>>not some random clocks, passed from the platform data. So, you should be
>>using explicit
>>clock names. In your platform data you can set whether a specific clock
>>should be used or
>>not, but not pass clock names down. Also you might want to consider using
>>devm_clk_get()
>>and be more careful with error handling.
>>
>OK, we will try to enhance it.
[Libin] Because there are some boards using mmp chip, and the clock names on
different board may be totally different. And also this is why the clock number
is not definite. To support more boards, the dynamic names are used instead of
the static names.
>
>>>
>>> static int mmpcam_probe(struct platform_device *pdev) { @@ -290,6
--
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
