RE: [PATCH RESEND v3 2/2] [media] V4L: atmel-isi: add clk_prepare()/clk_unprepare() functions
Hi, Guennadi
Thank you. I will send v4 version to apply your suggestion.
On Tuesday, January 10, 2012 7:29 PM, Guennadi Liakhovetski wrote:
Hi Josh
Right, sorry, I missed this one, I somehow developed an idea, that it
has
been merged into the original patch, adding ISI_MCK handling. Now, I
also
notice one detail, that we could improve:
On Tue, 10 Jan 2012, Josh Wu wrote:
Signed-off-by: Josh Wu josh...@atmel.com
Acked-by: Nicolas Ferre nicolas.fe...@atmel.com
---
Hi, Mauro
The first patch of this serie, [PATCH 1/2 v3] V4L: atmel-isi: add
code to enable/disable ISI_MCK clock, is already queued in media tree.
But this patch (the second one of this serie) is not acked yet. Would
it be ok to for you to ack this patch?
Best Regards,
Josh Wu
v2: made the label name to be consistent.
drivers/media/video/atmel-isi.c | 15 +++
1 files changed, 15 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/atmel-isi.c
b/drivers/media/video/atmel-isi.c
index ea4eef4..91ebcfb 100644
--- a/drivers/media/video/atmel-isi.c
+++ b/drivers/media/video/atmel-isi.c
@@ -922,7 +922,9 @@ static int __devexit atmel_isi_remove(struct
platform_device *pdev)
isi-fb_descriptors_phys);
iounmap(isi-regs);
+clk_unprepare(isi-mck);
clk_put(isi-mck);
+clk_unprepare(isi-pclk);
clk_put(isi-pclk);
kfree(isi);
@@ -955,6 +957,12 @@ static int __devinit atmel_isi_probe(struct
platform_device *pdev)
if (IS_ERR(pclk))
return PTR_ERR(pclk);
+ret = clk_prepare(pclk);
+if (ret) {
+clk_put(pclk);
+return ret;
Don't think it's a good idea here. You already have clk_put(pclk) on
the
error handling path below. So, just put a goto err_clk_prepare_pclk
here
and the respective error below.
Right. I'll fix it in v4.
Thanks
Guennadi
+}
+
isi = kzalloc(sizeof(struct atmel_isi), GFP_KERNEL);
if (!isi) {
ret = -ENOMEM;
@@ -978,6 +986,10 @@ static int __devinit atmel_isi_probe(struct
platform_device *pdev)
goto err_clk_get;
}
+ret = clk_prepare(isi-mck);
+if (ret)
+goto err_clk_prepare_mck;
+
/* Set ISI_MCK's frequency, it should be faster than pixel clock
*/
ret = clk_set_rate(isi-mck, pdata-mck_hz);
if (ret 0)
@@ -1059,10 +1071,13 @@ err_alloc_ctx:
isi-fb_descriptors_phys);
err_alloc_descriptors:
err_set_mck_rate:
+clk_unprepare(isi-mck);
+err_clk_prepare_mck:
clk_put(isi-mck);
err_clk_get:
kfree(isi);
err_alloc_isi:
+clk_unprepare(pclk);
clk_put(pclk);
return ret;
--
1.6.3.3
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
Best Regards,
Josh Wu
--
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 2/2] [media] V4L: atmel-isi: add clk_prepare()/clk_unprepare() functions
Hi, Guennadi
Would you acknowledge these two v3 patches and queue them for 3.3 merge
window? Thanks.
Best Regards,
Josh Wu
On Thursday, December 08, 2011 6:19 PM, Josh Wu wrote:
Signed-off-by: Josh Wu josh...@atmel.com
---
in v2 version, made the label name to be consistent
drivers/media/video/atmel-isi.c | 15 +++
1 files changed, 15 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/atmel-isi.c
b/drivers/media/video/atmel-isi.c
index ea4eef4..91ebcfb 100644
--- a/drivers/media/video/atmel-isi.c
+++ b/drivers/media/video/atmel-isi.c
@@ -922,7 +922,9 @@ static int __devexit atmel_isi_remove(struct
platform_device *pdev)
isi-fb_descriptors_phys);
iounmap(isi-regs);
+ clk_unprepare(isi-mck);
clk_put(isi-mck);
+ clk_unprepare(isi-pclk);
clk_put(isi-pclk);
kfree(isi);
@@ -955,6 +957,12 @@ static int __devinit atmel_isi_probe(struct
platform_device *pdev)
if (IS_ERR(pclk))
return PTR_ERR(pclk);
+ ret = clk_prepare(pclk);
+ if (ret) {
+ clk_put(pclk);
+ return ret;
+ }
+
isi = kzalloc(sizeof(struct atmel_isi), GFP_KERNEL);
if (!isi) {
ret = -ENOMEM;
@@ -978,6 +986,10 @@ static int __devinit atmel_isi_probe(struct
platform_device *pdev)
goto err_clk_get;
}
+ ret = clk_prepare(isi-mck);
+ if (ret)
+ goto err_clk_prepare_mck;
+
/* Set ISI_MCK's frequency, it should be faster than pixel clock
*/
ret = clk_set_rate(isi-mck, pdata-mck_hz);
if (ret 0)
@@ -1059,10 +1071,13 @@ err_alloc_ctx:
isi-fb_descriptors_phys);
err_alloc_descriptors:
err_set_mck_rate:
+ clk_unprepare(isi-mck);
+err_clk_prepare_mck:
clk_put(isi-mck);
err_clk_get:
kfree(isi);
err_alloc_isi:
+ clk_unprepare(pclk);
clk_put(pclk);
return ret;
--
1.6.3.3
--
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/2] [media] V4L: atmel-isi: add code to enable/disableISI_MCK clock
Hi, Russell King
On Wed, Dec 07, 2011 at 4:50 PM, Russell King wrote:
On Wed, Nov 30, 2011 at 06:06:43PM +0800, Josh Wu wrote:
+/* Get ISI_MCK, provided by programmable clock or external clock
*/
+isi-mck = clk_get(dev, isi_mck);
+if (IS_ERR_OR_NULL(isi-mck)) {
This should be IS_ERR()
So it means the clk_get() will never return NULL even when clk structure
is NULL in clk lookup entry. Right?
+dev_err(dev, Failed to get isi_mck\n);
+ret = isi-mck ? PTR_ERR(isi-mck) : -EINVAL;
ret = PTR_ERR(isi-mck);
Best Regards,
Josh Wu
--
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/2] [media] V4L: atmel-isi: add code toenable/disableISI_MCK clock
On Thursday, December 08, 2011 6:40AM, Russell King wrote:
On Wed, Dec 07, 2011 at 06:12:52PM +0800, Wu, Josh wrote:
Hi, Russell King
On Wed, Dec 07, 2011 at 4:50 PM, Russell King wrote:
On Wed, Nov 30, 2011 at 06:06:43PM +0800, Josh Wu wrote:
+ /* Get ISI_MCK, provided by programmable clock or external clock
*/
+ isi-mck = clk_get(dev, isi_mck);
+ if (IS_ERR_OR_NULL(isi-mck)) {
This should be IS_ERR()
So it means the clk_get() will never return NULL even when clk
structure
is NULL in clk lookup entry. Right?
It is not the drivers business to know whether NULL is valid or not.
clk_get() is defined to either return an error pointer, or a cookie
which the rest of the clk API must accept.
If an implementation decides that clk_get() can return NULL and deals
with that in the rest of the API (eg, to mean 'there is no clock but
don't fail for this') then drivers must not reject that.
If a driver rejects NULL then it is performing checks outside of the
definition of the clk API, and making assumptions about the nature of
valid cookies.
Thanks for the feedback. I will send v3 patch which will not check the
null return value.
Best Regards,
Josh Wu
--
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 2/2] [media] V4L: atmel-isi: add clk_prepare()/clk_unprepare() functions
Hi, Guennadi Thank you for explain the label name rules. I've sent the v2 version patch out. In v2 version I modified the code and make the label name consistent. On 12/06/2011 5:49PM, Guennadi Liakhovetski wrote: Hi Josh Thanks for the patch, but I'll ask you to fix the same thing in it, that I've fixed for you in the first patch in this series: On Wed, 30 Nov 2011, Josh Wu wrote: Signed-off-by: Josh Wu josh...@atmel.com --- drivers/media/video/atmel-isi.c | 17 - 1 files changed, 16 insertions(+), 1 deletions(-) diff --git a/drivers/media/video/atmel-isi.c b/drivers/media/video/atmel-isi.c index ea4eef4..5da4381 100644 --- a/drivers/media/video/atmel-isi.c +++ b/drivers/media/video/atmel-isi.c [snip] @@ -978,10 +986,14 @@ static int __devinit atmel_isi_probe(struct platform_device *pdev) goto err_clk_get; } +ret = clk_prepare(isi-mck); +if (ret) +goto err_set_mck_rate; + /* Set ISI_MCK's frequency, it should be faster than pixel clock */ ret = clk_set_rate(isi-mck, pdata-mck_hz); if (ret 0) -goto err_set_mck_rate; +goto err_unprepare_mck; isi-p_fb_descriptors = dma_alloc_coherent(pdev-dev, sizeof(struct fbd) * MAX_BUFFER_NUM, @@ -1058,11 +1070,14 @@ err_alloc_ctx: isi-p_fb_descriptors, isi-fb_descriptors_phys); err_alloc_descriptors: +err_unprepare_mck: +clk_unprepare(isi-mck); err_set_mck_rate: clk_put(isi-mck); err_clk_get: kfree(isi); err_alloc_isi: +clk_unprepare(pclk); clk_put(pclk); return ret; Please, use error label names consistently. As you can see, currently the driver uses the convention ret = do_something(); if (ret 0) goto err_do_something; i.e., the label is called after the operation, that has failed, not after the clean up step, that the control now has to jump to. Please, update your patch to also use this convention. Understand it now. Thank you. Thanks Guennadi Best Regards, Josh Wu -- 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 v4 1/3] [media] at91: add code to enable/disable ISI_MCK clock
Hi, Guennadi On Monday, October 17, 2011 11:04 PM, Guennadi Liakhovetski wrote: From: Josh Wu josh...@atmel.com This patch - add ISI_MCK clock enable/disable code. - change field name in isi_platform_data structure Signed-off-by: Josh Wu josh...@atmel.com [g.liakhovet...@gmx.de: fix label names] Signed-off-by: Guennadi Liakhovetski g.liakhovet...@gmx.de --- Josh, I missed a slight hick-up in the previous version of this your patch, so, I fixed it myself. Please confirm, that this version is ok with you. Thanks Guennadi I confirm that the patch is ok for me. Thank you. Best Regards, Josh Wu -- 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 2/2] at91: add Atmel ISI and ov2640 support on sam9m10/sam9g45 board.
On Thu, 22 Sep 2011, Guennadi wrote:
On Thu, 22 Sep 2011, Josh Wu wrote:
This patch
1. add ISI_MCK parent setting code when add ISI device.
2. add ov2640 support on board file.
3. define isi_mck clock in sam9g45 chip file.
Signed-off-by: Josh Wu josh.wu@x
---
arch/arm/mach-at91/at91sam9g45.c |3 +
arch/arm/mach-at91/at91sam9g45_devices.c | 105
+-
arch/arm/mach-at91/board-sam9m10g45ek.c | 85 -
arch/arm/mach-at91/include/mach/board.h |3 +-
Personally, I think, it would be better to separate this into two patches
at least: one for at91 core and one for the specific board, but that's up
to arch maintainers to decide.
You also want to patch arch/arm/mach-at91/at91sam9263_devices.c, don't
you?
As discussed in mail list, it will really break at91sam9263_devices.c's
compilation.
So I will fix this in 9263_device.c, and merge this fix with
mach-at91/include/mach/board.h change into one patch.
And other files as another patch.
4 files changed, 193 insertions(+), 3 deletions(-)
diff --git a/arch/arm/mach-at91/at91sam9g45.c
b/arch/arm/mach-at91/at91sam9g45.c
index e04c5fb..5e23d6d 100644
--- a/arch/arm/mach-at91/at91sam9g45.c
+++ b/arch/arm/mach-at91/at91sam9g45.c
@@ -201,6 +201,7 @@ static struct clk *periph_clocks[] __initdata = {
// irq0
};
+static struct clk pck1;
Hm, it really doesn't need any initialisation, not even for the .type
field? .type=0 doesn't seem to be valid.
This line is only a forward declaration. Since the real definition is behind
the code we use it.
It defined in later lines:
static struct clk pck1 = {
.name = pck1,
.pmc_mask = AT91_PMC_PCK1,
.type = CLK_TYPE_PROGRAMMABLE,
.id = 1,
};
static struct clk_lookup periph_clocks_lookups[] = {
/* One additional fake clock for ohci */
CLKDEV_CON_ID(ohci_clk, uhphs_clk),
@@ -215,6 +216,8 @@ static struct clk_lookup periph_clocks_lookups[] = {
CLKDEV_CON_DEV_ID(t0_clk, atmel_tcb.1, tcb0_clk),
CLKDEV_CON_DEV_ID(pclk, ssc.0, ssc0_clk),
CLKDEV_CON_DEV_ID(pclk, ssc.1, ssc1_clk),
+/* ISI_MCK, which is provided by programmable clock(PCK1) */
+CLKDEV_CON_DEV_ID(isi_mck, atmel_isi.0, pck1),
};
static struct clk_lookup usart_clocks_lookups[] = {
diff --git a/arch/arm/mach-at91/at91sam9g45_devices.c
b/arch/arm/mach-at91/at91sam9g45_devices.c
index 600bffb..82eeac8 100644
--- a/arch/arm/mach-at91/at91sam9g45_devices.c
+++ b/arch/arm/mach-at91/at91sam9g45_devices.c
@@ -16,7 +16,7 @@
#include linux/platform_device.h
#include linux/i2c-gpio.h
#include linux/atmel-mci.h
-
+#include linux/clk.h
#include linux/fb.h
#include video/atmel_lcdc.h
@@ -28,6 +28,8 @@
#include mach/at_hdmac.h
#include mach/atmel-mci.h
+#include media/atmel-isi.h
+
#include generic.h
@@ -863,6 +865,107 @@ void __init at91_add_device_ac97(struct
ac97c_platform_data *data)
void __init at91_add_device_ac97(struct ac97c_platform_data *data) {}
#endif
+/*
+ * Image Sensor Interface
+ * */
+#if defined(CONFIG_VIDEO_ATMEL_ISI) ||
defined(CONFIG_VIDEO_ATMEL_ISI_MODULE)
+static u64 isi_dmamask = DMA_BIT_MASK(32);
+static struct isi_platform_data isi_data;
+
+struct resource isi_resources[] = {
+[0] = {
+.start = AT91SAM9G45_BASE_ISI,
+.end= AT91SAM9G45_BASE_ISI + SZ_16K - 1,
+.flags = IORESOURCE_MEM,
+},
+[1] = {
+.start = AT91SAM9G45_ID_ISI,
+.end= AT91SAM9G45_ID_ISI,
+.flags = IORESOURCE_IRQ,
+},
+};
+
+static struct platform_device at91sam9g45_isi_device = {
+.name = atmel_isi,
+.id = 0,
+.dev= {
+.dma_mask = isi_dmamask,
+.coherent_dma_mask = DMA_BIT_MASK(32),
+.platform_data = isi_data,
+},
+.resource = isi_resources,
+.num_resources = ARRAY_SIZE(isi_resources),
+};
+
+static int __init isi_set_clk_parent(void)
+{
+struct clk *pck1;
+struct clk *plla;
+int ret;
+
+/* ISI_MCK is supplied by PCK1 - set parent for it. */
+pck1 = clk_get(NULL, pck1);
+if (IS_ERR(pck1)) {
+printk(KERN_ERR Failed to get PCK1\n);
+ret = PTR_ERR(pck1);
+goto err;
+}
+
+plla = clk_get(NULL, plla);
+if (IS_ERR(plla)) {
+printk(KERN_ERR Failed to get PLLA\n);
+ret = PTR_ERR(plla);
+goto err_pck1;
+}
+ret = clk_set_parent(pck1, plla);
+clk_put(plla);
+if (ret != 0) {
+printk(KERN_ERR Failed to set PCK1 parent\n);
+goto
RE: [PATCH v2] [media] at91: add code to initialize and manage the ISI_MCK for Atmel ISI driver.
Hi, Nicolas
On Friday, September 09, 2011 6:05 PM, Nicolas Ferre wrote:
Le 06/09/2011 08:54, Guennadi Liakhovetski :
On Tue, 6 Sep 2011, Josh Wu wrote:
This patch enable the configuration for ISI_MCK, which is provided by
programmable clock.
Signed-off-by: Josh Wu josh...@atmel.com
---
drivers/media/video/atmel-isi.c | 60
++-
include/media/atmel-isi.h |4 ++
2 files changed, 63 insertions(+), 1 deletions(-)
diff --git a/drivers/media/video/atmel-isi.c
b/drivers/media/video/atmel-isi.c
[..]
/*
-
--*/
+/* Initialize ISI_MCK clock, called by atmel_isi_probe() function */
+static int initialize_mck(struct platform_device *pdev,
+ struct atmel_isi *isi)
+{
+ struct device *dev = pdev-dev;
+ struct isi_platform_data *pdata = dev-platform_data;
+ struct clk *pck_parent;
+ int ret;
+
+ if (!strlen(pdata-pck_name) || !strlen(pdata-pck_parent_name))
+ return -EINVAL;
+
+ /* ISI_MCK is provided by PCK clock */
+ isi-mck = clk_get(dev, pdata-pck_name);
I think, it's still not what Russell meant. Look at
drivers/mmc/host/atmel-mci.c:
host-mck = clk_get(pdev-dev, mci_clk);
and in arch/arm/mach-at91/at91sam9g45.c they've got
CLKDEV_CON_DEV_ID(mci_clk, atmel_mci.0, mmc0_clk),
CLKDEV_CON_DEV_ID(mci_clk, atmel_mci.1, mmc1_clk),
where
#define CLKDEV_CON_DEV_ID(_con_id, _dev_id, _clk)\
{ \
.con_id = _con_id, \
.dev_id = _dev_id, \
.clk = _clk,\
}
I.e., in the device driver (mmc in this case) you only use the
(platform) device instance, whose dev_name(dev) is then matched
against one of clock lookups above, and a connection ID, which is used
in case your device is using more than one clock. In the ISI case,
your pck1 clock, that you seem to need here, doesn't have a clock
lookup object, so, you might have to add one, and then use its connection ID.
+ if (IS_ERR(isi-mck)) {
+ dev_err(dev, Failed to get PCK: %s\n, pdata-pck_name);
+ return PTR_ERR(isi-mck);
+ }
+
+ pck_parent = clk_get(dev, pdata-pck_parent_name);
+ if (IS_ERR(pck_parent)) {
+ ret = PTR_ERR(pck_parent);
+ dev_err(dev, Failed to get PCK parent: %s\n,
+ pdata-pck_parent_name);
+ goto err_init_mck;
+ }
+
+ ret = clk_set_parent(isi-mck, pck_parent);
I'm not entirely sure on this one, but as we had a similar situation
with clocks, we decided to extablish the clock hierarchy in the board
code, and only deal with the actual device clocks in the driver
itself. I.e., we moved all clk_set_parent() and setting up the parent clock
into the board.
And I do think, this makes more sense, than doing this in the driver,
not all users of this driver will need to manage the parent clock, right?
Exactly.
Josh, for the two comments by Guennadi above, you can take
sound/soc/atmel/sam9g20_wm8731.c as an example of using PCK and parent
clocks. You will also find how to use named clocks and how to set the
programmable clocks rate...
According to sam9g20_wm8731.c file, and combined Guennadi and J.C's advices,
I'd like to only add clk_set_rate() and enable/disble isi_mck code in ISI
driver.
Also I will define isi_mck clock in soc chip file(at91sam9g45.c).
Then I will move all setting up the parent clock code into the
at91sam9g45_devices.c, where there are code to add Atmel ISI device.
I will generate the new version patch for it soon.
+ clk_put(pck_parent);
+ if (ret)
+ goto err_init_mck;
+
+ ret = clk_set_rate(isi-mck, pdata-isi_mck_hz);
+ if (ret 0)
+ goto err_init_mck;
+
+ return 0;
+
+err_init_mck:
+ clk_put(isi-mck);
+ return ret;
+}
+
static int __devexit atmel_isi_remove(struct platform_device *pdev)
{
struct soc_camera_host *soc_host = to_soc_camera_host(pdev-dev);
@@ -897,6 +948,7 @@ static int __devexit atmel_isi_remove(struct
platform_device *pdev)
isi-fb_descriptors_phys);
iounmap(isi-regs);
+ clk_put(isi-mck);
clk_put(isi-pclk);
kfree(isi);
[..]
Best regards,
--
Nicolas Ferre
Best Regards,
Josh Wu
--
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] at91: add Atmel ISI and ov2640 support on m10/g45 board.
On 09/03/2011 2:22 AM Jean-Christophe PLAGNIOL-VILLARD wrote:
#include asm/setup.h
#include asm/mach-types.h
@@ -194,6 +197,95 @@ static void __init ek_add_device_nand(void)
at91_add_device_nand(ek_nand_data);
}
+/*
+ * ISI
+ */
+#if defined(CONFIG_VIDEO_ATMEL_ISI) ||
defined(CONFIG_VIDEO_ATMEL_ISI_MODULE)
+static struct isi_platform_data __initdata isi_data = {
+.frate = ISI_CFG1_FRATE_CAPTURE_ALL,
+.has_emb_sync = 0,
+.emb_crc_sync = 0,
+.hsync_act_low = 0,
+.vsync_act_low = 0,
+.pclk_act_falling = 0,
+/* to use codec and preview path simultaneously */
+.isi_full_mode = 1,
+.data_width_flags = ISI_DATAWIDTH_8 | ISI_DATAWIDTH_10,
+};
+
+static void __init isi_set_clk(void)
+{
+struct clk *pck1;
+struct clk *plla;
+
+pck1 = clk_get(NULL, pck1);
+plla = clk_get(NULL, plla);
+
+clk_set_parent(pck1, plla);
+clk_set_rate(pck1, 2500);
+clk_enable(pck1);
you must not enable the clock always
you must enable it just when you need it
and manage the clock at the board level really so so
I see, I will move such clock code to atmel_isi.c driver and add clock name,
clock frequence to isi_platform_data structure in next version.
Thanks.
Best Regards,
Josh Wu
Best Regards,
J.
--
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] [media] at91: add code to initialize and manage theISI_MCK for Atmel ISI driver.
Hi, Russell
On 09/05/2011 6:34 PM Russell King wrote:
On Mon, Sep 05, 2011 at 06:29:53PM +0800, Josh Wu wrote:
+static int initialize_mck(struct atmel_isi *isi,
+struct isi_platform_data *pdata)
+{
+int ret;
+struct clk *pck_parent;
+
+if (!strlen(pdata-pck_name) || !strlen(pdata-pck_parent_name))
+return -EINVAL;
+
+/* ISI_MCK is provided by PCK clock */
+isi-mck = clk_get(NULL, pdata-pck_name);
No, this is not how you use the clk API. You do not pass clock names via
platform data.
You pass clk_get() the struct device. You then pass clk_get() a
_connection id_ on that _device_ if you have more than one struct clk
associated with the _device_. You then use clkdev to associate the
struct device plus the connection id with the appropriate struct clk.
I think I missed the struct device when called clk_get(). I will fix it in v2
version.
Best Regards,
Josh Wu
--
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: Using atmel-isi for direct output on framebuffer ?
Hi, Thomas On Thu, 1 Sep 2011, Thomas Petazzoni wrote: Hello Josh, I am currently looking at V4L2 and your atmel-isi driver for an AT91 based platform on which I would like the ISI interface to capture the image from a camera and have this image directly output in RGB format at a specific location on the screen (so that it can be nicely integrated into a Qt application for example). At the moment, I grab frames from the V4L2 device to userspace, do the YUV - RGB conversion manually in my application, and then displays the converted frame on the framebuffer thanks to normal Qt painting mechanisms. This works, but obviously consumes a lot of CPU. From the AT91 datasheet, I understand that the ISI interface is capable of doing the YUV - RGB conversion and is also capable of outputting the frame at some location in the framebuffer, but I don't see how to use this capability with the Linux V4L2 and framebuffer infrastructures. Is this possible ? If so, could you provide some pointers or starting points to get me started ? If not, what is missing in the driver ? My understanding is that you want to use Atmel ISI to output RGB data then work with framebuffer. So yes, it is possible. Since current atmel_isi.c only uses its codec path to output YUV data. So first need add RGB format support in isi_camera_get_formats(). Then you have two choices to enable RGB output of ISI: 1. Enable isi's preview path(DMA, interrupts) to convert YUV to RGB. 2. Or still use codec path but don't need add much ISI code, just set camera sensor(if it support RGB565 output) to output RGB565 data for ISI, then what the data ISI output now should be RGB565 format. But in this way you cannot do any scale. For V4L2_CAP_VIDEO_OVERLAY type driver, I don't know much about that. Best Regards, Josh Wu Thanks a lot, Thomas -- Thomas Petazzoni, Free Electrons Kernel, drivers, real-time and embedded Linux development, consulting, training and support. http://free-electrons.com -- 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 v3 RESEND] media: vb2: change queue initialization order
Hi, Marek
On Monday, August 29, 2011 7:58 PM, Marek Szyprowski wrote:
This patch changes the order of operations during stream on call. Now the
buffers are first queued to the driver and then the start_streaming method
is called.
This resolves the most common case when the driver needs to know buffer
addresses to enable dma engine and start streaming. Additional parameter
to start_streaming method have been added to simplify drivers code. The
driver are now obliged to check if the number of queued buffers is high
enough to enable hardware streaming. If not - it can return an error. In
such case all the buffers that have been pre-queued are invalidated.
This patch also updates all videobuf2 clients to work properly with the
changed order of operations.
Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
Signed-off-by: Kyungmin Park kyungmin.p...@samsung.com
CC: Pawel Osciak pa...@osciak.com
CC: Guennadi Liakhovetski g.liakhovet...@gmx.de
CC: Hans Verkuil hverk...@xs4all.nl
CC: Tomasz Stanislawski t.stanisl...@samsung.com
CC: Sylwester Nawrocki s.nawro...@samsung.com
CC: Kamil Debski k.deb...@samsung.com
CC: Jonathan Corbet cor...@lwn.net
CC: Josh Wu josh...@atmel.com
CC: Hans de Goede hdego...@redhat.com
CC: Paul Mundt let...@linux-sh.org
---
Hello,
This is yet another version of the patch that introduces significant
changes in the vb2 streamon operation. I've decided to remove the
additional parameter to buf_queue callback and added a few cleanups here
and there. This patch also includes an update for all vb2 clients.
Please check if my change didn't break anything.
(I'm sorry for spamming, but previous version had wrong code for
atmel-isi driver).
Best regards
--
Marek Szyprowski
Samsung Poland RD Center
For atmel-isi.c,
Tested-by: Josh Wu josh...@atmel.com
Best Regards,
Josh Wu
drivers/media/video/atmel-isi.c | 20 --
drivers/media/video/marvell-ccic/mcam-core.c |6 +-
drivers/media/video/pwc/pwc-if.c |2 +-
drivers/media/video/s5p-fimc/fimc-capture.c | 65 +++---
drivers/media/video/s5p-mfc/s5p_mfc_dec.c|2 +-
drivers/media/video/s5p-mfc/s5p_mfc_enc.c|2 +-
drivers/media/video/s5p-tv/mixer.h |2 -
drivers/media/video/s5p-tv/mixer_video.c | 22 +++---
drivers/media/video/videobuf2-core.c | 97
--
drivers/media/video/vivi.c |2 +-
include/media/videobuf2-core.h | 17 -
11 files changed, 131 insertions(+), 106 deletions(-)
diff --git a/drivers/media/video/atmel-isi.c b/drivers/media/video/atmel-isi.c
index 7e1d789..774715d 100644
--- a/drivers/media/video/atmel-isi.c
+++ b/drivers/media/video/atmel-isi.c
@@ -404,12 +404,13 @@ static void buffer_queue(struct vb2_buffer *vb)
if (isi-active == NULL) {
isi-active = buf;
- start_dma(isi, buf);
+ if (vb2_is_streaming(vb-vb2_queue))
+ start_dma(isi, buf);
}
spin_unlock_irqrestore(isi-lock, flags);
}
-static int start_streaming(struct vb2_queue *vq)
+static int start_streaming(struct vb2_queue *vq, unsigned int count)
{
struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
struct soc_camera_host *ici = to_soc_camera_host(icd-parent);
@@ -431,17 +432,26 @@ static int start_streaming(struct vb2_queue *vq)
ret = wait_event_interruptible(isi-vsync_wq,
isi-state != ISI_STATE_IDLE);
if (ret)
- return ret;
+ goto err;
- if (isi-state != ISI_STATE_READY)
- return -EIO;
+ if (isi-state != ISI_STATE_READY) {
+ ret = -EIO;
+ goto err;
+ }
spin_lock_irq(isi-lock);
isi-state = ISI_STATE_WAIT_SOF;
isi_writel(isi, ISI_INTDIS, ISI_SR_VSYNC);
+ if (count)
+ start_dma(isi, isi-active);
spin_unlock_irq(isi-lock);
return 0;
+err:
+ isi-active = NULL;
+ isi-sequence = 0;
+ INIT_LIST_HEAD(isi-video_buffer_list);
+ return ret;
}
/* abort streaming and wait for last buffer */
diff --git a/drivers/media/video/marvell-ccic/mcam-core.c
b/drivers/media/video/marvell-ccic/mcam-core.c
index 7abe503..1141b97 100644
--- a/drivers/media/video/marvell-ccic/mcam-core.c
+++ b/drivers/media/video/marvell-ccic/mcam-core.c
@@ -940,12 +940,14 @@ static void mcam_vb_wait_finish(struct vb2_queue *vq)
/*
* These need to be called with the mutex held from vb2
*/
[snip]
struct vb2_ops {
int (*queue_setup)(struct vb2_queue *q, unsigned int *num_buffers,
@@ -219,7 +228,7 @@ struct vb2_ops {
int (*buf_finish)(struct vb2_buffer *vb);
void (*buf_cleanup)(struct vb2_buffer *vb);
- int (*start_streaming)(struct vb2_queue *q);
+ int (*start_streaming)(struct vb2_queue *q
RE: [PATCH v3] [media] at91: add Atmel Image Sensor Interface (ISI) support
Hi, Guennadi
On Friday, June 03, 2011, Guennadi Liakhovetski wrote
On Fri, 3 Jun 2011, Josh Wu wrote:
This patch is to enable Atmel Image Sensor Interface (ISI) driver support.
- Using soc-camera framework with videobuf2 dma-contig allocator
- Supporting video streaming of YUV packed format
- Tested on AT91SAM9M10G45-EK with OV2640
Signed-off-by: Josh Wu josh...@atmel.com
---
base on branch staging/for_v3.0
Modified in V3:
refined the interrupt handling code.
added a list to manage the allocated descriptors.
removed redundant code in isi_camera_set_bus_param().
return error when platform data is not provided.
drivers/media/video/Kconfig |8 +
drivers/media/video/Makefile|1 +
drivers/media/video/atmel-isi.c | 1074
+++
include/media/atmel-isi.h | 119 +
4 files changed, 1202 insertions(+), 0 deletions(-)
create mode 100644 drivers/media/video/atmel-isi.c
create mode 100644 include/media/atmel-isi.h
diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index bb53de7..93d1098 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -952,6 +952,14 @@ config VIDEO_SAMSUNG_S5P_FIMC
To compile this driver as a module, choose M here: the
module will be called s5p-fimc.
+config VIDEO_ATMEL_ISI
+tristate ATMEL Image Sensor Interface (ISI) support
+depends on VIDEO_DEV SOC_CAMERA ARCH_AT91
+select VIDEOBUF2_DMA_CONTIG
+---help---
+ This module makes the ATMEL Image Sensor Interface available
+ as a v4l2 device.
+
config VIDEO_S5P_MIPI_CSIS
tristate Samsung S5P and EXYNOS4 MIPI CSI receiver driver
depends on VIDEO_V4L2 PM_RUNTIME PLAT_S5P VIDEO_V4L2_SUBDEV_API
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index f0fecd6..d9833f4 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -166,6 +166,7 @@ obj-$(CONFIG_VIDEO_PXA27x) += pxa_camera.o
obj-$(CONFIG_VIDEO_SH_MOBILE_CSI2) += sh_mobile_csi2.o
obj-$(CONFIG_VIDEO_SH_MOBILE_CEU) += sh_mobile_ceu_camera.o
obj-$(CONFIG_VIDEO_OMAP1) += omap1_camera.o
+obj-$(CONFIG_VIDEO_ATMEL_ISI) += atmel-isi.o
obj-$(CONFIG_VIDEO_SAMSUNG_S5P_FIMC)+= s5p-fimc/
diff --git a/drivers/media/video/atmel-isi.c
b/drivers/media/video/atmel-isi.c
new file mode 100644
index 000..a020bd3
--- /dev/null
+++ b/drivers/media/video/atmel-isi.c
@@ -0,0 +1,1074 @@
+/*
+ * Copyright (c) 2011 Atmel Corporation
+ * Josh Wu, josh...@atmel.com
+ *
+ * Based on previous work by Lars Haring, lars.har...@atmel.com
+ * and Sedji Gaouaou
+ * Based on the bttv driver for Bt848 with respective copyright holders
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include linux/clk.h
+#include linux/completion.h
+#include linux/delay.h
+#include linux/fs.h
+#include linux/init.h
+#include linux/interrupt.h
+#include linux/kernel.h
+#include linux/module.h
+#include linux/platform_device.h
+#include linux/slab.h
+
+#include media/atmel-isi.h
+#include media/soc_camera.h
+#include media/soc_mediabus.h
+#include media/videobuf2-dma-contig.h
+
+#define MAX_BUFFER_NUM 32
+#define MAX_SUPPORT_WIDTH 2048
+#define MAX_SUPPORT_HEIGHT 2048
+#define VID_LIMIT_BYTES (16 * 1024 * 1024)
+#define MIN_FRAME_RATE 15
+#define FRAME_INTERVAL_MILLI_SEC(1000 / MIN_FRAME_RATE)
+
+/* ISI states */
+enum {
+ISI_STATE_IDLE = 0,
+ISI_STATE_READY,
+ISI_STATE_WAIT_SOF,
+};
+
+/* Frame buffer descriptor */
+struct fbd {
+/* Physical address of the frame buffer */
+u32 fb_address;
+/* DMA Control Register(only in HISI2) */
+u32 dma_ctrl;
+/* Physical address of the next fbd */
+u32 next_fbd_address;
+};
+
+static void set_dma_ctrl(struct fbd *fb_desc, u32 ctrl)
+{
+fb_desc-dma_ctrl = ctrl;
+}
+
+struct isi_dma_desc {
+struct list_head list;
+struct fbd *p_fbd;
+u32 fbd_phys;
+};
+
+/* Frame buffer data */
+struct frame_buffer {
+struct vb2_buffer vb;
+struct fbd *p_fb_desc;
+u32 fb_desc_phys;
Why don't you replace the above two members with struct isi_dma_desc *?
Then you also don't need to scan the DMA descriptor array in
.buf_cleanup().
Will fix in v4. This change makes code simpler.
+struct list_head list;
+};
+
+struct atmel_isi {
+/* Protects the access of variables shared with the ISR */
+spinlock_t lock;
+void __iomem*regs;
+
+int sequence;
+/* State of the ISI module in capturing mode */
+int
RE: [PATCH v2] [media] at91: add Atmel Image Sensor Interface (ISI) support
Hi, Guennadi
Guennadi Liakhovetski wrote on May 29, 2011 4:25 AM
Hi Josh
Thanks for the update. A general note: I much prefer the new IO accessors and
register names and values - thanks for changing that!
Thank you for the reviewing. I also think this version is clearer. :)
Base on the suggestion from you and J.C, I will send out the version 3 soon.
On Fri, 27 May 2011, Josh Wu wrote:
This patch is to enable Atmel Image Sensor Interface (ISI) driver support.
- Using soc-camera framework with videobuf2 dma-contig allocator
- Supporting video streaming of YUV packed format
- Tested on AT91SAM9M10G45-EK with OV2640
Signed-off-by: Josh Wu josh...@atmel.com
---
Modified in V2 patch:
[snip]
+
+#include linux/clk.h
+#include linux/completion.h
+#include linux/fs.h
+#include linux/init.h
+#include linux/interrupt.h
+#include linux/kernel.h
You had linux/module.h here in v1, I think, it is needed for various
MODULE_AUTHOR() etc. macros. Please, re-add.
I will add it.
+#include linux/platform_device.h
+#include linux/slab.h
+#include linux/delay.h
+
+#include mach/board.h
+#include mach/cpu.h
I still don't understand, why you need these two. If unneeded, please, remove.
I forgot to remove it. I'll remove this.
+
+#include media/videobuf2-dma-contig.h #include media/soc_camera.h
+#include media/soc_mediabus.h #include media/atmel-isi.h
Alphabetic order here too, please.
I will change the order.
+
+#define MAX_BUFFER_NUMS 32
NUM above doesn't need an S at the end - it's just a number of buffers,
not numbers.
sure, I will fix it.
+#define MAX_SUPPORT_WIDTH 2048
+#define MAX_SUPPORT_HEIGHT 2048
+#define VID_LIMIT_BYTES (16 * 1024 * 1024)
+#define MIN_FRAME_RATE 15
+#define FRAME_INTERVAL_MILLI_SEC(1000 / MIN_FRAME_RATE)
+
+/* ISI states */
+enum {
+ISI_STATE_IDLE = 0,
+ISI_STATE_READY,
+ISI_STATE_WAIT_SOF,
+};
+
+/* Frame buffer descriptor */
+struct fbd {
+/* Physical address of the frame buffer */
+u32 fb_address;
+/* DMA Control Register(only in HISI2) */
+u32 dma_ctrl;
Ok, several reviewers, including myself, asked you to remove #ifdef here, but
at least I didn't realise at that moment, that this struct describes
hareware-specific memory layout. So, how is it supposed to work now on
platforms, that don't have this field, i.e., !CONFIG_ARCH_AT91SAM9G45 and
!CONFIG_ARCH_AT91SAM9X5? Or are they not supported yet? Maybe you could
define two structs - later, when you also support other CPUs, and use a
union or something else?
This dma_ctrl is hardware related memory layout. In the ISI_V1 hardware
(AT91SAM9263, AVR32, etc), there is no dma_ctrl in the descriptor. In the
future I prefer to add another ISI_V1 fbd structure and a function to handle
these two structures according to hardware. But in this version I think ISI_V1
hardware platform is not supported.
+/* Physical address of the next fbd */
+u32 next_fbd_address;
+};
+
+static void set_dma_ctrl(struct fbd *fb_desc, u32 ctrl) {
+fb_desc-dma_ctrl = ctrl;
+}
+
+/* Frame buffer data */
+struct frame_buffer {
+struct vb2_buffer vb;
+struct fbd *p_fb_desc;
+u32 fb_desc_phys;
+struct list_head list;
+};
+
+struct atmel_isi {
+/* Protects the access of variables shared with the ISR */
+spinlock_t lock;
+void __iomem*regs;
+
+int sequence;
+/* State of the ISI module in capturing mode */
+int state;
+
+/* Capture/streaming wait queue for waiting for SOF */
+wait_queue_head_t capture_wq;
+
+struct vb2_alloc_ctx*alloc_ctx;
+
+/* Allocate descriptors for dma buffer use */
+struct fbd *p_fb_descriptors;
+u32 fb_descriptors_phys;
+int index_fb_desc;
+
+struct completion isi_complete;
You don't need to prefix members in structs, especially since you don't do
this with others. Just leave complete or something similar.
I will change the name.
[snip]
+
+if (!buf-p_fb_desc) {
+/* Initialize the dma descriptor */
+buf-p_fb_desc = isi-p_fb_descriptors[isi-index_fb_desc];
+buf-fb_desc_phys = isi-fb_descriptors_phys +
+isi-index_fb_desc * sizeof(struct fbd);
+
+buf-p_fb_desc-fb_address = vb2_dma_contig_plane_paddr(vb, 0);
+buf-p_fb_desc-next_fbd_address = 0;
+set_dma_ctrl(buf-p_fb_desc, ISI_DMA_CTRL_WB);
+isi-index_fb_desc++;
+if (isi-index_fb_desc MAX_BUFFER_NUMS) {
+dev_err(icd-dev.parent, Not enough dma
descriptors.\n);
Don't you have to check overflow _before_ using the index
RE: [PATCH v2] [media] at91: add Atmel Image Sensor Interface (ISI) support
Hi, Jean-Christophe
Thank you for the review.
Jean-Christophe PLAGNIOL-VILLARD wrote on Friday, May 27, 2011 8:06 PM:
+/* ISI interrupt service routine */
+static irqreturn_t isi_interrupt(int irq, void *dev_id) {
+struct atmel_isi *isi = dev_id;
+u32 status, mask, pending;
+irqreturn_t ret = IRQ_NONE;
+
+spin_lock(isi-lock);
+
+status = isi_readl(isi, ISI_STATUS);
+mask = isi_readl(isi, ISI_INTMASK);
+pending = status mask;
+
+if (pending ISI_CTRL_SRST) {
+complete(isi-isi_complete);
+isi_writel(isi, ISI_INTDIS, ISI_CTRL_SRST);
+ret = IRQ_HANDLED;
+}
+if (pending ISI_CTRL_DIS) {
+complete(isi-isi_complete);
+isi_writel(isi, ISI_INTDIS, ISI_CTRL_DIS);
+ret = IRQ_HANDLED;
+}
no else here?
+
+if (pending ISI_SR_VSYNC) {
+switch (isi-state) {
+case ISI_STATE_IDLE:
+isi-state = ISI_STATE_READY;
+wake_up_interruptible(isi-capture_wq);
+break;
+}
really switch here?
I will remove the switch here.
I think this part of IRQ handling code need to refine a little bit. The SRST
and DIS_DONE is more independent. And other interrupts can compose together.
Following is the latest code, I think is more reasonable.
if (pending ISI_CTRL_SRST) {
complete(isi-complete);
isi_writel(isi, ISI_INTDIS, ISI_CTRL_SRST);
ret = IRQ_HANDLED;
} else if (pending ISI_CTRL_DIS) {
complete(isi-complete);
isi_writel(isi, ISI_INTDIS, ISI_CTRL_DIS);
ret = IRQ_HANDLED;
} else {
if ((pending ISI_SR_VSYNC)
(isi-state == ISI_STATE_IDLE)) {
isi-state = ISI_STATE_READY;
wake_up_interruptible(isi-vsync_wq);
ret = IRQ_HANDLED;
}
if (likely(pending ISI_SR_CXFR_DONE))
ret = atmel_isi_handle_streaming(isi);
}
+} else if (likely(pending ISI_SR_CXFR_DONE)) {
+ret = atmel_isi_handle_streaming(isi);
+}
+
+spin_unlock(isi-lock);
+
+return ret;
+}
+
+#define WAIT_ISI_RESET 1
+#define WAIT_ISI_DISABLE0
+static int atmel_isi_wait_status(int wait_reset, struct atmel_isi
+*isi)
I thinkhave teh atmel_isti first parameter is better
I will fix it.
+{
+unsigned long timeout;
+/*
+ * The reset or disable will only succeed if we have a
+ * pixel clock from the camera.
+ */
+init_completion(isi-isi_complete);
+
+if (wait_reset) {
+isi_writel(isi, ISI_INTEN, ISI_CTRL_SRST);
+isi_writel(isi, ISI_CTRL, ISI_CTRL_SRST);
+} else {
+isi_writel(isi, ISI_INTEN, ISI_CTRL_DIS);
+isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
+}
+
+timeout = wait_for_completion_timeout(isi-isi_complete,
+msecs_to_jiffies(100));
+if (timeout == 0)
+return -ETIMEDOUT;
+
+return 0;
+}
+
+/* --
+Videobuf operations
+
+--*/
+static int queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
+unsigned int *nplanes, unsigned long sizes[],
+void *alloc_ctxs[])
+{
+struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
+struct soc_camera_host *ici = to_soc_camera_host(icd-dev.parent);
+struct atmel_isi *isi = ici-priv;
+unsigned long size;
+int ret, bytes_per_line;
+
+/* Reset ISI */
+ret = atmel_isi_wait_status(WAIT_ISI_RESET, isi);
+if (ret 0) {
+dev_err(icd-dev.parent, Reset ISI timed out\n);
+return ret;
+}
+/* Disable all interrupts */
+isi_writel(isi, ISI_INTDIS, ~0UL);
+
+bytes_per_line = soc_mbus_bytes_per_line(icd-user_width,
+icd-current_fmt-host_fmt);
+
+if (bytes_per_line 0)
+return bytes_per_line;
+
+size = bytes_per_line * icd-user_height;
+
+if (*nbuffers == 0)
+*nbuffers = MAX_BUFFER_NUMS;
+if (*nbuffers MAX_BUFFER_NUMS)
else here
I will add it.
+*nbuffers = MAX_BUFFER_NUMS;
+
+if (size * *nbuffers VID_LIMIT_BYTES)
+*nbuffers = VID_LIMIT_BYTES / size;
+
+*nplanes = 1;
+sizes[0] = size;
+alloc_ctxs[0] = isi-alloc_ctx;
+
+isi-sequence = 0;
+isi-active = NULL;
+
+dev_dbg(icd-dev.parent, %s, count=%d, size=%ld\n, __func__,
+*nbuffers, size);
+
+return 0;
+}
+
+static int buffer_init(struct vb2_buffer *vb) {
+struct frame_buffer *buf = container_of(vb, struct frame_buffer,
+vb);
+
+buf-p_fb_desc = NULL;
+buf-fb_desc_phys = 0;
memset 0?
OK.
+INIT_LIST_HEAD(buf-list);
RE: [PATCH v2] [media] at91: add Atmel Image Sensor Interface (ISI) support
Hi, Arnd On Friday, May 27, 2011 9:50 PM, Arnd Bergmann wrote On Friday 27 May 2011, Josh Wu wrote: This patch is to enable Atmel Image Sensor Interface (ISI) driver support. - Using soc-camera framework with videobuf2 dma-contig allocator - Supporting video streaming of YUV packed format - Tested on AT91SAM9M10G45-EK with OV2640 Signed-off-by: Josh Wu josh...@atmel.com Looks good to me now. Acked-by: Arnd Bergmann a...@arndb.de Thank you very much. I'll send a version 3 code. Best Regards, Josh Wu -- 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] [media] at91: add Atmel Image Sensor Interface (ISI)support
Hi, Guennadi Sorry to answer the question so later, From: Guennadi Liakhovetski Sent: Thursday, May 12, 2011 5:32 PM On Thu, 12 May 2011, Wu, Josh wrote: Hi, Russell From: Russell King - ARM Linux [mailto:li...@arm.linux.org.uk] Sent: Thursday, May 12, 2011 3:47 PM On Thu, May 12, 2011 at 03:42:18PM +0800, Josh Wu wrote: +err_alloc_isi: + clk_disable(pclk); clk_put() ? Ok, will be fixed in V2 patch. Thanks. You might wait with v2 until I find time to review your patch. Will take a couple of days, I think. A general question, though: how compatible is this driver with the AVR32? Now from the information I got from AVR team, The AVR32 is not compatible with ISI driver patch v2. Since AVR32 use the previous version of ISI IP core. (AT91SAM9M10 using ISI_V2 IP, which has different hardware registers from V1 IP core, such like dma control, etc) But I think in future we will add ISI V1 IP support since AT91SAM9263 also using ISI V1 IP core. Best Regards, Josh Wu -- 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] [media] at91: add Atmel Image Sensor Interface (ISI) support
Hi, JC
+struct atmel_isi;
do we really this here?
Not really. I'll remove this.
+
[snip]
if VIDEO_CAPTURE_DRIVERS VIDEO_V4L2
+config VIDEO_ATMEL_ISI
+tristate ATMEL Image Sensor Interface (ISI) support
+depends on VIDEO_DEV SOC_CAMERA
depends on AT91 if the drivers is at91 specific or avr32 otherwise
I'll add that. I think now it is only supported AT91. I am not sure for the
AVR32 part
+select VIDEOBUF2_DMA_CONTIG
+default n
it's n by default please remove
I'll change that.
[snip]
+
+/* Frame buffer descriptor
+ * Used by the ISI module as a linked list for the DMA controller.
+ */
+struct fbd {
+/* Physical address of the frame buffer */
+u32 fb_address;
+#if defined(CONFIG_ARCH_AT91SAM9G45) ||\
+defined(CONFIG_ARCH_AT91SAM9X5)
+/* DMA Control Register(only in HISI2) */
+u32 dma_ctrl;
+#endif
no ifdef in the struct
I'll remove this #if. I think for the non-HISI2 version, like AT91SAM9263, we
should define another FBD structure which not includes dma_ctrl.
+/* Physical address of the next fbd */
+u32 next_fbd_address;
+};
+
+#if defined(CONFIG_ARCH_AT91SAM9G45) ||\
+defined(CONFIG_ARCH_AT91SAM9X5)
+static void set_dma_ctrl(struct fbd *fb_desc, u32 ctrl) {
+fb_desc-dma_ctrl = ctrl;
+}
+#else
+static void set_dma_ctrl(struct fbd *fb_desc, u32 ctrl) { } #endif
no ifdef here also as we want to have multi soc support
I'll remove this #if also.
[snip]
+/* State of the ISI module in capturing mode */
+int state;
+
+/* Capture/streaming wait queue for waiting for SOF */
+wait_queue_head_t capture_wq;
+
+struct v4l2_device v4l2_dev;
+
+struct vb2_alloc_ctx*alloc_ctx;
+
+struct clk *pclk;
+struct platform_device *pdev;
do you really need to store the pdev?
I'll remove this. It is no use.
+unsigned intirq;
+
+struct isi_platform_data*pdata;
+unsigned long platform_flags;
+
+struct list_headvideo_buffer_list;
+struct frame_buffer *active;
+
+struct soc_camera_device*icd;
+struct soc_camera_host soc_host;
+};
+
+static int configure_geometry(struct atmel_isi *isi, u32 width,
+u32 height, enum v4l2_mbus_pixelcode code) {
+u32 cfg2, cr, ctrl;
+
+cr = 0;
please move this in default
I'll remove this cr line. Seems it is not needed. cr will be initialized by the
following code.
[snip]
+
+size = bytes_per_line * icd-user_height;
+
+if (0 == *nbuffers)
please invert the test
I'll fix it.
+*nbuffers = MAX_BUFFER_NUMS;
+if (*nbuffers MAX_BUFFER_NUMS)
+*nbuffers = MAX_BUFFER_NUMS;
+
+while (size * *nbuffers vid_limit * 1024 * 1024)
+(*nbuffers)--;
+
+*nplanes = 1;
+sizes[0] = size;
+alloc_ctxs[0] = dev-alloc_ctx;
+
+dev-sequence = 0;
+dev-active = NULL;
+
+dev_dbg(icd-dev.parent, %s, count=%d, size=%ld\n, __func__,
+*nbuffers, size);
+
+return 0;
+}
+
+
+static void start_dma(struct atmel_isi *isi, struct frame_buffer
+*buffer) {
+u32 ctrl, cfg1;
please add ine ligne here
OK. I'll change that.
+ctrl = isi_readl(isi, V2_CTRL);
+cfg1 = isi_readl(isi, V2_CFG1);
+/* Enable irq: cxfr for the codec path, pxfr for the preview path */
+isi_writel(isi, V2_INTEN,
+ISI_BIT(V2_CXFR_DONE) | ISI_BIT(V2_PXFR_DONE));
+
+/* Enable codec path */
+ctrl |= ISI_BIT(V2_CDC);
+/* Check if already in a frame */
+while (isi_readl(isi, V2_STATUS) ISI_BIT(V2_CDC))
+cpu_relax();
no timeout?
I'll add time out code and test it.
+
+/* Write the address of the first frame buffer in the C_ADDR reg
+* write the address of the first descriptor(link list of buffer)
+* in the C_DSCR reg, and enable dma channel.
+*/
+isi_writel(isi, V2_DMA_C_DSCR, (__pa((buffer-fb_desc;
+isi_writel(isi, V2_DMA_C_CTRL,
+ISI_BIT(V2_DMA_FETCH) | ISI_BIT(V2_DMA_DONE));
+isi_writel(isi, V2_DMA_CHER, ISI_BIT(V2_DMA_C_CH_EN));
+
+/* Enable linked list */
+cfg1 |= ISI_BF(V2_FRATE, isi-pdata-frate) | ISI_BIT(V2_DISCR);
+
+/* Enable ISI module*/
+ctrl |= ISI_BIT(V2_ENABLE);
+isi_writel(isi, V2_CTRL, ctrl);
+isi_writel(isi, V2_CFG1, cfg1);
+}
+
+
+/* abort streaming and wait for last buffer */ static int
+stop_streaming(struct vb2_queue *vq) {
+struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
+struct soc_camera_host *ici = to_soc_camera_host(icd-dev.parent);
+struct atmel_isi *isi = ici-priv;
+
+spin_lock_irq(isi-lock);
+isi-still_capture = 0;
+isi-active = NULL;
+
+while (isi_readl(isi, V2_STATUS) ISI_BIT(V2_CDC))
+
RE: [PATCH] [media] at91: add Atmel Image Sensor Interface (ISI) support
On Friday, May 13, 2011 5:25 AM, Ryan Mallon wrote
On 05/12/2011 07:42 PM, Josh Wu wrote:
This patch is to enable Atmel Image Sensor Interface (ISI) driver support.
- Using soc-camera framework with videobuf2 dma-contig allocator
- Supporting video streaming of YUV packed format
- Tested on AT91SAM9M10G45-EK with OV2640
Hi Josh,
Thansk for doing this. Overall the patch looks really good. A few
comments below.
Hi, Ryan
Thank you for the comments.
Signed-off-by: Josh Wu josh...@atmel.com
---
base on branch staging/for_v2.6.40
arch/arm/mach-at91/include/mach/at91_isi.h | 454
drivers/media/video/Kconfig| 10 +
drivers/media/video/Makefile |1 +
drivers/media/video/atmel-isi.c| 1089
4 files changed, 1554 insertions(+), 0 deletions(-)
create mode 100644 arch/arm/mach-at91/include/mach/at91_isi.h
create mode 100644 drivers/media/video/atmel-isi.c
[snip]
+
+/* Bit manipulation macros */
+#define ISI_BIT(name) \
+(1 ISI_##name##_OFFSET)
+#define ISI_BF(name, value) \
+(((value) ((1 ISI_##name##_SIZE) - 1)) \
+ ISI_##name##_OFFSET)
+#define ISI_BFEXT(name, value) \
+(((value) ISI_##name##_OFFSET) \
+ ((1 ISI_##name##_SIZE) - 1))
+#define ISI_BFINS(name, value, old) \
+(((old) ~(((1 ISI_##name##_SIZE) - 1) \
+ ISI_##name##_OFFSET))\
+ | ISI_BF(name, value))
I really dislike this kind of register access magic. Not sure how others
feel about it. These macros are really ugly.
I understand this. So I will try to find a better way (static inline function)
to solve this. :)
+/* Register access macros */
+#define isi_readl(port, reg)\
+__raw_readl((port)-regs + ISI_##reg)
+#define isi_writel(port, reg, value)\
+__raw_writel((value), (port)-regs + ISI_##reg)
If the token pasting stuff gets dropped then these can be static inline
functions which is preferred.
sure, I'll try.
[snip]
+#define ISI_GS_2PIX_PER_WORD0x00
+#define ISI_GS_1PIX_PER_WORD0x01
+u8 pixfmt;
+u8 sfd;
+u8 sld;
+u8 thmask;
+#define ISI_BURST_4_8_160x00
+#define ISI_BURST_8_16 0x01
+#define ISI_BURST_160x02
+u8 frate;
+#define ISI_FRATE_DIV_2 0x01
+#define ISI_FRATE_DIV_3 0x02
+#define ISI_FRATE_DIV_4 0x03
+#define ISI_FRATE_DIV_5 0x04
+#define ISI_FRATE_DIV_6 0x05
+#define ISI_FRATE_DIV_7 0x06
+#define ISI_FRATE_DIV_8 0x07
+};
Might need some comments in this structure so that board file writers
know what each of the fields are for.
I think this part will be change a little bit. Also I will add the updated
comments.
+
+#endif /* __AT91_ISI_H__ */
diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index d61414e..eae6005 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -80,6 +80,16 @@ menuconfig VIDEO_CAPTURE_DRIVERS
Some of those devices also supports FM radio.
if VIDEO_CAPTURE_DRIVERS VIDEO_V4L2
+config VIDEO_ATMEL_ISI
+tristate ATMEL Image Sensor Interface (ISI) support
+depends on VIDEO_DEV SOC_CAMERA
Depends on AT91/AVR32?
I think I will use AT91
[snip]
+
+#include media/videobuf2-dma-contig.h
+#include media/soc_camera.h
+#include media/soc_mediabus.h
+
+#define ATMEL_ISI_VERSION KERNEL_VERSION(1, 0, 0)
Do we really need this version?
Since we need set a version for v4l2_capability.version in function
isi_camera_querycap(). So we use this macro.
How about this?
static int isi_camera_querycap(struct soc_camera_host *ici,
struct v4l2_capability *cap)
{
strcpy(cap-driver, atmel-isi);
strcpy(cap-card, Atmel Image Sensor Interface);
cap-version = KERNEL_VERSION(1, 0, 0);
cap-capabilities = (V4L2_CAP_VIDEO_CAPTURE |
V4L2_CAP_STREAMING);
return 0;
}
+#define MAX_BUFFER_NUMS 32
+#define MAX_SUPPORT_WIDTH 2048
+#define MAX_SUPPORT_HEIGHT 2048
+
+static unsigned int vid_limit = 16;
This never gets changed so it can become a const/define. The value is
MB, which is not clear from the name, and it gets multiplied out to
bytes in its only usage, so maybe:
#define VID_LIMIT_BYTES (16 * 1024 * 1024)
Your code is good. I will change according to your suggestion.
+
+enum isi_buffer_state {
+ISI_BUF_NEEDS_INIT,
+ISI_BUF_PREPARED,
+};
Aren't there v4l2 constants for this already?
VIDEOBUF_NEEDS_INIT,
VIDEOBUF_PREPARED,
Just reuse those.
I checked the code, above constants are used
RE: [PATCH] [media] at91: add Atmel Image Sensor Interface (ISI)support
Hi, Russell From: Russell King - ARM Linux [mailto:li...@arm.linux.org.uk] Sent: Thursday, May 12, 2011 3:47 PM On Thu, May 12, 2011 at 03:42:18PM +0800, Josh Wu wrote: +err_alloc_isi: +clk_disable(pclk); clk_put() ? Ok, will be fixed in V2 patch. Thanks. Best Regards, Josh Wu -- 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] [media] at91: add Atmel Image Sensor Interface (ISI)support
From: Guennadi Liakhovetski [mailto:g.liakhovet...@gmx.de] Sent: Thursday, May 12, 2011 5:32 PM On Thu, 12 May 2011, Wu, Josh wrote: Hi, Russell From: Russell King - ARM Linux [mailto:li...@arm.linux.org.uk] Sent: Thursday, May 12, 2011 3:47 PM On Thu, May 12, 2011 at 03:42:18PM +0800, Josh Wu wrote: +err_alloc_isi: + clk_disable(pclk); clk_put() ? Ok, will be fixed in V2 patch. Thanks. You might wait with v2 until I find time to review your patch. Will take a couple of days, I think. A general question, though: how compatible is this driver with the AVR32? That is ok. For AVR32, I think I need time to check with AVR team. I will update the status when I got more information. Best Regards, Josh Wu -- 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] [media] at91: add Atmel Image Sensor Interface (ISI)support
On Thursday, May 12, 2011 5:35 PM, Russell King wrote:
A few more points...
+static int __init atmel_isi_probe(struct platform_device *pdev)
Should be __devinit otherwise you'll have section errors.
Ok, will be fixed in V2 patch.
+{
+unsigned int irq;
+struct atmel_isi *isi;
+struct clk *pclk;
+struct resource *regs;
+int ret;
+struct device *dev = pdev-dev;
+struct isi_platform_data *pdata;
+struct soc_camera_host *soc_host;
+
+regs = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+if (!regs)
+return -ENXIO;
+
+pclk = clk_get(pdev-dev, isi_clk);
+if (IS_ERR(pclk))
+return PTR_ERR(pclk);
+
+clk_enable(pclk);
Return value of clk_enable() should be checked.
Yes. I will add code to check the return value in V2 patch.
+
+isi = kzalloc(sizeof(struct atmel_isi), GFP_KERNEL);
+if (!isi) {
+ret = -ENOMEM;
[...]
+isi_writel(isi, V2_CTRL, ISI_BIT(V2_DIS));
+/* Check if module disable */
+while (isi_readl(isi, V2_STATUS) ISI_BIT(V2_DIS))
+cpu_relax();
+
+irq = platform_get_irq(pdev, 0);
This should also be checked.
Ditto, thank you.
--
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
