* [PATCH 07/13] atmel-isi: move hw code into isi_hw_initialize()
2016-01-18 12:52 ` [PATCH 06/13] atmel-isi: check ISI_SR's flags by polling instead of interrupt Josh Wu
@ 2016-01-18 12:52 ` Josh Wu
2016-01-24 18:09 ` Guennadi Liakhovetski
2016-01-18 12:52 ` [PATCH 08/13] atmel-isi: remove the function set_dma_ctrl() as it just use once Josh Wu
` (7 subsequent siblings)
8 siblings, 1 reply; 29+ messages in thread
From: Josh Wu @ 2016-01-18 12:52 UTC (permalink / raw)
To: linux-arm-kernel
That make hw operation code separate with general code.
Also since reset action can be failed, so add a return value for
isi_hw_initialze().
Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
---
drivers/media/platform/soc_camera/atmel-isi.c | 34 +++++++++++++++++----------
1 file changed, 21 insertions(+), 13 deletions(-)
diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index 4ddc309..ed4d04b 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -203,10 +203,27 @@ static int isi_hw_wait_status(struct atmel_isi *isi, int status_flag,
return 0;
}
-static void isi_hw_initialize(struct atmel_isi *isi)
+static int isi_hw_initialize(struct atmel_isi *isi)
{
u32 common_flags = isi->bus_param;
u32 cfg1 = 0;
+ int ret;
+
+ /* Reset ISI */
+ isi_writel(isi, ISI_CTRL, ISI_CTRL_SRST);
+
+ /* Check Reset status */
+ ret = isi_hw_wait_status(isi, ISI_CTRL_SRST, 500);
+ if (ret) {
+ dev_err(isi->soc_host.icd->parent, "Reset ISI timed out\n");
+ return ret;
+ }
+
+ /* Disable all interrupts */
+ isi_writel(isi, ISI_INTDIS, (u32)~0UL);
+
+ /* Clear any pending interrupt */
+ isi_readl(isi, ISI_STATUS);
/* set bus param for ISI */
if (common_flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)
@@ -229,6 +246,8 @@ static void isi_hw_initialize(struct atmel_isi *isi)
isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
isi_writel(isi, ISI_CFG1, cfg1);
+
+ return 0;
}
static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
@@ -453,27 +472,16 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
pm_runtime_get_sync(ici->v4l2_dev.dev);
- /* Reset ISI */
- isi_writel(isi, ISI_CTRL, ISI_CTRL_SRST);
-
- /* Check Reset status */
- ret = isi_hw_wait_status(isi, ISI_CTRL_SRST, 500);
+ ret = isi_hw_initialize(isi);
if (ret) {
- dev_err(icd->parent, "Reset ISI timed out\n");
pm_runtime_put(ici->v4l2_dev.dev);
return ret;
}
- /* Disable all interrupts */
- isi_writel(isi, ISI_INTDIS, (u32)~0UL);
-
- isi_hw_initialize(isi);
configure_geometry(isi, icd->user_width, icd->user_height,
icd->current_fmt);
spin_lock_irq(&isi->lock);
- /* Clear any pending interrupt */
- isi_readl(isi, ISI_STATUS);
if (count)
start_dma(isi, isi->active);
--
1.9.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH 07/13] atmel-isi: move hw code into isi_hw_initialize()
2016-01-18 12:52 ` [PATCH 07/13] atmel-isi: move hw code into isi_hw_initialize() Josh Wu
@ 2016-01-24 18:09 ` Guennadi Liakhovetski
2016-01-26 14:07 ` Josh Wu
0 siblings, 1 reply; 29+ messages in thread
From: Guennadi Liakhovetski @ 2016-01-24 18:09 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, 18 Jan 2016, Josh Wu wrote:
> That make hw operation code separate with general code.
>
> Also since reset action can be failed, so add a return value for
> isi_hw_initialze().
>
> Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
> ---
>
> drivers/media/platform/soc_camera/atmel-isi.c | 34 +++++++++++++++++----------
> 1 file changed, 21 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
> index 4ddc309..ed4d04b 100644
> --- a/drivers/media/platform/soc_camera/atmel-isi.c
> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> @@ -203,10 +203,27 @@ static int isi_hw_wait_status(struct atmel_isi *isi, int status_flag,
> return 0;
> }
>
> -static void isi_hw_initialize(struct atmel_isi *isi)
> +static int isi_hw_initialize(struct atmel_isi *isi)
> {
> u32 common_flags = isi->bus_param;
> u32 cfg1 = 0;
> + int ret;
> +
> + /* Reset ISI */
> + isi_writel(isi, ISI_CTRL, ISI_CTRL_SRST);
> +
> + /* Check Reset status */
> + ret = isi_hw_wait_status(isi, ISI_CTRL_SRST, 500);
You could also remove the superfluous space while at it.
Thanks
Guennadi
> + if (ret) {
> + dev_err(isi->soc_host.icd->parent, "Reset ISI timed out\n");
> + return ret;
> + }
> +
> + /* Disable all interrupts */
> + isi_writel(isi, ISI_INTDIS, (u32)~0UL);
> +
> + /* Clear any pending interrupt */
> + isi_readl(isi, ISI_STATUS);
>
> /* set bus param for ISI */
> if (common_flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)
> @@ -229,6 +246,8 @@ static void isi_hw_initialize(struct atmel_isi *isi)
>
> isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
> isi_writel(isi, ISI_CFG1, cfg1);
> +
> + return 0;
> }
>
> static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
> @@ -453,27 +472,16 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
>
> pm_runtime_get_sync(ici->v4l2_dev.dev);
>
> - /* Reset ISI */
> - isi_writel(isi, ISI_CTRL, ISI_CTRL_SRST);
> -
> - /* Check Reset status */
> - ret = isi_hw_wait_status(isi, ISI_CTRL_SRST, 500);
> + ret = isi_hw_initialize(isi);
> if (ret) {
> - dev_err(icd->parent, "Reset ISI timed out\n");
> pm_runtime_put(ici->v4l2_dev.dev);
> return ret;
> }
> - /* Disable all interrupts */
> - isi_writel(isi, ISI_INTDIS, (u32)~0UL);
> -
> - isi_hw_initialize(isi);
>
> configure_geometry(isi, icd->user_width, icd->user_height,
> icd->current_fmt);
>
> spin_lock_irq(&isi->lock);
> - /* Clear any pending interrupt */
> - isi_readl(isi, ISI_STATUS);
>
> if (count)
> start_dma(isi, isi->active);
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 29+ messages in thread* [PATCH 07/13] atmel-isi: move hw code into isi_hw_initialize()
2016-01-24 18:09 ` Guennadi Liakhovetski
@ 2016-01-26 14:07 ` Josh Wu
0 siblings, 0 replies; 29+ messages in thread
From: Josh Wu @ 2016-01-26 14:07 UTC (permalink / raw)
To: linux-arm-kernel
Hi, Guennadi
Thanks for the reivew.
2016-01-25 2:09 GMT+08:00 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> On Mon, 18 Jan 2016, Josh Wu wrote:
>
>> That make hw operation code separate with general code.
>>
>> Also since reset action can be failed, so add a return value for
>> isi_hw_initialze().
>>
>> Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
>> ---
>>
>> drivers/media/platform/soc_camera/atmel-isi.c | 34 +++++++++++++++++----------
>> 1 file changed, 21 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
>> index 4ddc309..ed4d04b 100644
>> --- a/drivers/media/platform/soc_camera/atmel-isi.c
>> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
>> @@ -203,10 +203,27 @@ static int isi_hw_wait_status(struct atmel_isi *isi, int status_flag,
>> return 0;
>> }
>>
>> -static void isi_hw_initialize(struct atmel_isi *isi)
>> +static int isi_hw_initialize(struct atmel_isi *isi)
>> {
>> u32 common_flags = isi->bus_param;
>> u32 cfg1 = 0;
>> + int ret;
>> +
>> + /* Reset ISI */
>> + isi_writel(isi, ISI_CTRL, ISI_CTRL_SRST);
>> +
>> + /* Check Reset status */
>> + ret = isi_hw_wait_status(isi, ISI_CTRL_SRST, 500);
>
> You could also remove the superfluous space while at it.
sure, I'll fix the duplicated space here.
Best Regards,
Josh Wu
>
> Thanks
> Guennadi
>
>> + if (ret) {
>> + dev_err(isi->soc_host.icd->parent, "Reset ISI timed out\n");
>> + return ret;
>> + }
>> +
>> + /* Disable all interrupts */
>> + isi_writel(isi, ISI_INTDIS, (u32)~0UL);
>> +
>> + /* Clear any pending interrupt */
>> + isi_readl(isi, ISI_STATUS);
>>
>> /* set bus param for ISI */
>> if (common_flags & V4L2_MBUS_HSYNC_ACTIVE_LOW)
>> @@ -229,6 +246,8 @@ static void isi_hw_initialize(struct atmel_isi *isi)
>>
>> isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
>> isi_writel(isi, ISI_CFG1, cfg1);
>> +
>> + return 0;
>> }
>>
>> static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
>> @@ -453,27 +472,16 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
>>
>> pm_runtime_get_sync(ici->v4l2_dev.dev);
>>
>> - /* Reset ISI */
>> - isi_writel(isi, ISI_CTRL, ISI_CTRL_SRST);
>> -
>> - /* Check Reset status */
>> - ret = isi_hw_wait_status(isi, ISI_CTRL_SRST, 500);
>> + ret = isi_hw_initialize(isi);
>> if (ret) {
>> - dev_err(icd->parent, "Reset ISI timed out\n");
>> pm_runtime_put(ici->v4l2_dev.dev);
>> return ret;
>> }
>> - /* Disable all interrupts */
>> - isi_writel(isi, ISI_INTDIS, (u32)~0UL);
>> -
>> - isi_hw_initialize(isi);
>>
>> configure_geometry(isi, icd->user_width, icd->user_height,
>> icd->current_fmt);
>>
>> spin_lock_irq(&isi->lock);
>> - /* Clear any pending interrupt */
>> - isi_readl(isi, ISI_STATUS);
>>
>> if (count)
>> start_dma(isi, isi->active);
>> --
>> 1.9.1
>>
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 08/13] atmel-isi: remove the function set_dma_ctrl() as it just use once
2016-01-18 12:52 ` [PATCH 06/13] atmel-isi: check ISI_SR's flags by polling instead of interrupt Josh Wu
2016-01-18 12:52 ` [PATCH 07/13] atmel-isi: move hw code into isi_hw_initialize() Josh Wu
@ 2016-01-18 12:52 ` Josh Wu
2016-01-18 12:52 ` [PATCH 09/13] atmel-isi: add a function start_isi() Josh Wu
` (6 subsequent siblings)
8 siblings, 0 replies; 29+ messages in thread
From: Josh Wu @ 2016-01-18 12:52 UTC (permalink / raw)
To: linux-arm-kernel
From: Josh Wu <josh.wu@atmel.com>
Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
---
drivers/media/platform/soc_camera/atmel-isi.c | 7 +------
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index ed4d04b..e1ad18f 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -46,11 +46,6 @@ struct 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;
@@ -385,7 +380,7 @@ static int buffer_prepare(struct vb2_buffer *vb)
desc->p_fbd->fb_address =
vb2_dma_contig_plane_dma_addr(vb, 0);
desc->p_fbd->next_fbd_address = 0;
- set_dma_ctrl(desc->p_fbd, ISI_DMA_CTRL_WB);
+ desc->p_fbd->dma_ctrl = ISI_DMA_CTRL_WB;
buf->p_dma_desc = desc;
}
--
1.9.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH 09/13] atmel-isi: add a function start_isi()
2016-01-18 12:52 ` [PATCH 06/13] atmel-isi: check ISI_SR's flags by polling instead of interrupt Josh Wu
2016-01-18 12:52 ` [PATCH 07/13] atmel-isi: move hw code into isi_hw_initialize() Josh Wu
2016-01-18 12:52 ` [PATCH 08/13] atmel-isi: remove the function set_dma_ctrl() as it just use once Josh Wu
@ 2016-01-18 12:52 ` Josh Wu
2016-01-18 12:52 ` [PATCH 10/13] atmel-isi: reuse start_dma() function in isi interrupt handler Josh Wu
` (5 subsequent siblings)
8 siblings, 0 replies; 29+ messages in thread
From: Josh Wu @ 2016-01-18 12:52 UTC (permalink / raw)
To: linux-arm-kernel
Since start_dma() has code which is to start isi, so move such code into
a new function: start_isi().
Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
---
drivers/media/platform/soc_camera/atmel-isi.c | 32 +++++++++++++++------------
1 file changed, 18 insertions(+), 14 deletions(-)
diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index e1ad18f..0e42171 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -245,6 +245,20 @@ static int isi_hw_initialize(struct atmel_isi *isi)
return 0;
}
+static void start_isi(struct atmel_isi *isi)
+{
+ u32 ctrl;
+
+ /* cxfr for the codec path, pxfr for the preview path */
+ isi_writel(isi, ISI_INTEN,
+ ISI_SR_CXFR_DONE | ISI_SR_PXFR_DONE);
+
+ /* Enable ISI */
+ ctrl = ISI_CTRL_EN |
+ (isi->enable_preview_path ? 0 : ISI_CTRL_CDC);
+ isi_writel(isi, ISI_CTRL, ctrl);
+}
+
static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
{
if (isi->active) {
@@ -403,12 +417,6 @@ static void buffer_cleanup(struct vb2_buffer *vb)
static void start_dma(struct atmel_isi *isi, struct frame_buffer *buffer)
{
- u32 ctrl;
-
- /* Enable irq: cxfr for the codec path, pxfr for the preview path */
- isi_writel(isi, ISI_INTEN,
- ISI_SR_CXFR_DONE | ISI_SR_PXFR_DONE);
-
/* Check if already in a frame */
if (!isi->enable_preview_path) {
if (isi_readl(isi, ISI_STATUS) & ISI_CTRL_CDC) {
@@ -429,13 +437,6 @@ static void start_dma(struct atmel_isi *isi, struct frame_buffer *buffer)
isi_writel(isi, ISI_DMA_CHER, ISI_DMA_CHSR_P_CH);
}
- /* Enable ISI */
- ctrl = ISI_CTRL_EN;
-
- if (!isi->enable_preview_path)
- ctrl |= ISI_CTRL_CDC;
-
- isi_writel(isi, ISI_CTRL, ctrl);
}
static void buffer_queue(struct vb2_buffer *vb)
@@ -478,8 +479,11 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
spin_lock_irq(&isi->lock);
- if (count)
+ if (count) {
start_dma(isi, isi->active);
+ start_isi(isi);
+ }
+
spin_unlock_irq(&isi->lock);
return 0;
--
1.9.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH 10/13] atmel-isi: reuse start_dma() function in isi interrupt handler
2016-01-18 12:52 ` [PATCH 06/13] atmel-isi: check ISI_SR's flags by polling instead of interrupt Josh Wu
` (2 preceding siblings ...)
2016-01-18 12:52 ` [PATCH 09/13] atmel-isi: add a function start_isi() Josh Wu
@ 2016-01-18 12:52 ` Josh Wu
2016-01-18 12:52 ` [PATCH 11/13] atmel-isi: add hw_uninitialize() in stop_streaming() Josh Wu
` (4 subsequent siblings)
8 siblings, 0 replies; 29+ messages in thread
From: Josh Wu @ 2016-01-18 12:52 UTC (permalink / raw)
To: linux-arm-kernel
Reuse start_dma() in interrupt handler function to avoid duplication.
Also we need to move start_dma() function up in the file.
Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
---
drivers/media/platform/soc_camera/atmel-isi.c | 62 +++++++++++----------------
1 file changed, 25 insertions(+), 37 deletions(-)
diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index 0e42171..c1a8698 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -259,6 +259,30 @@ static void start_isi(struct atmel_isi *isi)
isi_writel(isi, ISI_CTRL, ctrl);
}
+static void start_dma(struct atmel_isi *isi, struct frame_buffer *buffer)
+{
+ /* Check if already in a frame */
+ if (!isi->enable_preview_path) {
+ if (isi_readl(isi, ISI_STATUS) & ISI_CTRL_CDC) {
+ dev_err(isi->soc_host.icd->parent, "Already in frame handling.\n");
+ return;
+ }
+
+ isi_writel(isi, ISI_DMA_C_DSCR,
+ (u32)buffer->p_dma_desc->fbd_phys);
+ isi_writel(isi, ISI_DMA_C_CTRL,
+ ISI_DMA_CTRL_FETCH | ISI_DMA_CTRL_DONE);
+ isi_writel(isi, ISI_DMA_CHER, ISI_DMA_CHSR_C_CH);
+ } else {
+ isi_writel(isi, ISI_DMA_P_DSCR,
+ (u32)buffer->p_dma_desc->fbd_phys);
+ isi_writel(isi, ISI_DMA_P_CTRL,
+ ISI_DMA_CTRL_FETCH | ISI_DMA_CTRL_DONE);
+ isi_writel(isi, ISI_DMA_CHER, ISI_DMA_CHSR_P_CH);
+ }
+
+}
+
static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
{
if (isi->active) {
@@ -277,19 +301,7 @@ static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
/* start next dma frame. */
isi->active = list_entry(isi->video_buffer_list.next,
struct frame_buffer, list);
- if (!isi->enable_preview_path) {
- isi_writel(isi, ISI_DMA_C_DSCR,
- (u32)isi->active->p_dma_desc->fbd_phys);
- isi_writel(isi, ISI_DMA_C_CTRL,
- ISI_DMA_CTRL_FETCH | ISI_DMA_CTRL_DONE);
- isi_writel(isi, ISI_DMA_CHER, ISI_DMA_CHSR_C_CH);
- } else {
- isi_writel(isi, ISI_DMA_P_DSCR,
- (u32)isi->active->p_dma_desc->fbd_phys);
- isi_writel(isi, ISI_DMA_P_CTRL,
- ISI_DMA_CTRL_FETCH | ISI_DMA_CTRL_DONE);
- isi_writel(isi, ISI_DMA_CHER, ISI_DMA_CHSR_P_CH);
- }
+ start_dma(isi, isi->active);
}
return IRQ_HANDLED;
}
@@ -415,30 +427,6 @@ static void buffer_cleanup(struct vb2_buffer *vb)
list_add(&buf->p_dma_desc->list, &isi->dma_desc_head);
}
-static void start_dma(struct atmel_isi *isi, struct frame_buffer *buffer)
-{
- /* Check if already in a frame */
- if (!isi->enable_preview_path) {
- if (isi_readl(isi, ISI_STATUS) & ISI_CTRL_CDC) {
- dev_err(isi->soc_host.icd->parent, "Already in frame handling.\n");
- return;
- }
-
- isi_writel(isi, ISI_DMA_C_DSCR,
- (u32)buffer->p_dma_desc->fbd_phys);
- isi_writel(isi, ISI_DMA_C_CTRL,
- ISI_DMA_CTRL_FETCH | ISI_DMA_CTRL_DONE);
- isi_writel(isi, ISI_DMA_CHER, ISI_DMA_CHSR_C_CH);
- } else {
- isi_writel(isi, ISI_DMA_P_DSCR,
- (u32)buffer->p_dma_desc->fbd_phys);
- isi_writel(isi, ISI_DMA_P_CTRL,
- ISI_DMA_CTRL_FETCH | ISI_DMA_CTRL_DONE);
- isi_writel(isi, ISI_DMA_CHER, ISI_DMA_CHSR_P_CH);
- }
-
-}
-
static void buffer_queue(struct vb2_buffer *vb)
{
struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
--
1.9.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH 11/13] atmel-isi: add hw_uninitialize() in stop_streaming()
2016-01-18 12:52 ` [PATCH 06/13] atmel-isi: check ISI_SR's flags by polling instead of interrupt Josh Wu
` (3 preceding siblings ...)
2016-01-18 12:52 ` [PATCH 10/13] atmel-isi: reuse start_dma() function in isi interrupt handler Josh Wu
@ 2016-01-18 12:52 ` Josh Wu
2016-01-18 12:52 ` [PATCH 11/13] atmel-isi: add hw_uninitialize() Josh Wu
` (3 subsequent siblings)
8 siblings, 0 replies; 29+ messages in thread
From: Josh Wu @ 2016-01-18 12:52 UTC (permalink / raw)
To: linux-arm-kernel
add new function: hw_uninitialize() and call it when stop_streaming().
Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
---
drivers/media/platform/soc_camera/atmel-isi.c | 46 +++++++++++++++------------
1 file changed, 26 insertions(+), 20 deletions(-)
diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index c1a8698..7d2e952 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -245,6 +245,31 @@ static int isi_hw_initialize(struct atmel_isi *isi)
return 0;
}
+static void isi_hw_uninitialize(struct atmel_isi *isi)
+{
+ int ret;
+
+ if (!isi->enable_preview_path) {
+ /* Wait until the end of the current frame. */
+ ret = isi_hw_wait_status(isi, ISI_CTRL_CDC,
+ FRAME_INTERVAL_MILLI_SEC);
+ if (ret)
+ dev_err(isi->soc_host.icd->parent, "Timeout waiting for finishing codec request\n");
+ }
+
+ /* Disable interrupts */
+ isi_writel(isi, ISI_INTDIS,
+ ISI_SR_CXFR_DONE | ISI_SR_PXFR_DONE);
+
+ /* Disable ISI and wait for it is done */
+ isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
+
+ /* Check Reset status */
+ ret = isi_hw_wait_status(isi, ISI_CTRL_DIS, 500);
+ if (ret)
+ dev_err(isi->soc_host.icd->parent, "Disable ISI timed out\n");
+}
+
static void start_isi(struct atmel_isi *isi)
{
u32 ctrl;
@@ -484,7 +509,6 @@ static void stop_streaming(struct vb2_queue *vq)
struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
struct atmel_isi *isi = ici->priv;
struct frame_buffer *buf, *node;
- int ret = 0;
spin_lock_irq(&isi->lock);
isi->active = NULL;
@@ -495,25 +519,7 @@ static void stop_streaming(struct vb2_queue *vq)
}
spin_unlock_irq(&isi->lock);
- if (!isi->enable_preview_path) {
- /* Wait until the end of the current frame. */
- ret = isi_hw_wait_status(isi, ISI_CTRL_CDC,
- FRAME_INTERVAL_MILLI_SEC);
- if (ret)
- dev_err(icd->parent, "Timeout waiting for finishing codec request\n");
- }
-
- /* Disable interrupts */
- isi_writel(isi, ISI_INTDIS,
- ISI_SR_CXFR_DONE | ISI_SR_PXFR_DONE);
-
- /* Disable ISI and wait for it is done */
- isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
-
- /* Check Reset status */
- ret = isi_hw_wait_status(isi, ISI_CTRL_DIS, 500);
- if (ret)
- dev_err(icd->parent, "Disable ISI timed out\n");
+ isi_hw_uninitialize(isi);
pm_runtime_put(ici->v4l2_dev.dev);
}
--
1.9.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH 11/13] atmel-isi: add hw_uninitialize()
2016-01-18 12:52 ` [PATCH 06/13] atmel-isi: check ISI_SR's flags by polling instead of interrupt Josh Wu
` (4 preceding siblings ...)
2016-01-18 12:52 ` [PATCH 11/13] atmel-isi: add hw_uninitialize() in stop_streaming() Josh Wu
@ 2016-01-18 12:52 ` Josh Wu
2016-01-18 12:52 ` [PATCH 12/13] atmel-isi: use union for the fbd (frame buffer descriptor) Josh Wu
` (2 subsequent siblings)
8 siblings, 0 replies; 29+ messages in thread
From: Josh Wu @ 2016-01-18 12:52 UTC (permalink / raw)
To: linux-arm-kernel
add new function: hw_uninitialize() and call it when stop_streaming().
Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
---
drivers/media/platform/soc_camera/atmel-isi.c | 46 +++++++++++++++------------
1 file changed, 26 insertions(+), 20 deletions(-)
diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index b789523..843102f 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -245,6 +245,31 @@ static int isi_hw_initialize(struct atmel_isi *isi)
return 0;
}
+static void isi_hw_uninitialize(struct atmel_isi *isi)
+{
+ int ret;
+
+ if (!isi->enable_preview_path) {
+ /* Wait until the end of the current frame. */
+ ret = isi_hw_wait_status(isi, ISI_CTRL_CDC,
+ FRAME_INTERVAL_MILLI_SEC);
+ if (ret)
+ dev_err(isi->soc_host.icd->parent, "Timeout waiting for finishing codec request\n");
+ }
+
+ /* Disable interrupts */
+ isi_writel(isi, ISI_INTDIS,
+ ISI_SR_CXFR_DONE | ISI_SR_PXFR_DONE);
+
+ /* Disable ISI and wait for it is done */
+ isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
+
+ /* Check Reset status */
+ ret = isi_hw_wait_status(isi, ISI_CTRL_DIS, 500);
+ if (ret)
+ dev_err(isi->soc_host.icd->parent, "Disable ISI timed out\n");
+}
+
static void start_isi(struct atmel_isi *isi)
{
u32 ctrl;
@@ -484,7 +509,6 @@ static void stop_streaming(struct vb2_queue *vq)
struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
struct atmel_isi *isi = ici->priv;
struct frame_buffer *buf, *node;
- int ret = 0;
spin_lock_irq(&isi->lock);
isi->active = NULL;
@@ -495,25 +519,7 @@ static void stop_streaming(struct vb2_queue *vq)
}
spin_unlock_irq(&isi->lock);
- if (!isi->enable_preview_path) {
- /* Wait until the end of the current frame. */
- ret = isi_hw_wait_status(isi, ISI_CTRL_CDC,
- FRAME_INTERVAL_MILLI_SEC);
- if (ret)
- dev_err(icd->parent, "Timeout waiting for finishing codec request\n");
- }
-
- /* Disable interrupts */
- isi_writel(isi, ISI_INTDIS,
- ISI_SR_CXFR_DONE | ISI_SR_PXFR_DONE);
-
- /* Disable ISI and wait for it is done */
- isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
-
- /* Check Reset status */
- ret = isi_hw_wait_status(isi, ISI_CTRL_DIS, 500);
- if (ret)
- dev_err(icd->parent, "Disable ISI timed out\n");
+ isi_hw_uninitialize(isi);
pm_runtime_put(ici->v4l2_dev.dev);
}
--
1.9.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH 12/13] atmel-isi: use union for the fbd (frame buffer descriptor)
2016-01-18 12:52 ` [PATCH 06/13] atmel-isi: check ISI_SR's flags by polling instead of interrupt Josh Wu
` (5 preceding siblings ...)
2016-01-18 12:52 ` [PATCH 11/13] atmel-isi: add hw_uninitialize() Josh Wu
@ 2016-01-18 12:52 ` Josh Wu
2016-01-24 19:31 ` Guennadi Liakhovetski
2016-01-18 12:52 ` [PATCH 13/13] atmel-isi: use an hw_data structure according compatible string Josh Wu
2016-01-24 16:58 ` [PATCH 06/13] atmel-isi: check ISI_SR's flags by polling instead of interrupt Guennadi Liakhovetski
8 siblings, 1 reply; 29+ messages in thread
From: Josh Wu @ 2016-01-18 12:52 UTC (permalink / raw)
To: linux-arm-kernel
From: Josh Wu <josh.wu@atmel.com>
This way, we can easy to add other type of fbd for new hardware.
Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
---
drivers/media/platform/soc_camera/atmel-isi.c | 33 ++++++++++++++++++---------
1 file changed, 22 insertions(+), 11 deletions(-)
diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index 7d2e952..b4c1f38 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -37,7 +37,7 @@
#define FRAME_INTERVAL_MILLI_SEC (1000 / MIN_FRAME_RATE)
/* Frame buffer descriptor */
-struct fbd {
+struct fbd_isi_v2 {
/* Physical address of the frame buffer */
u32 fb_address;
/* DMA Control Register(only in HISI2) */
@@ -46,9 +46,13 @@ struct fbd {
u32 next_fbd_address;
};
+union fbd {
+ struct fbd_isi_v2 fbd_isi;
+};
+
struct isi_dma_desc {
struct list_head list;
- struct fbd *p_fbd;
+ union fbd *p_fbd;
dma_addr_t fbd_phys;
};
@@ -69,7 +73,7 @@ struct atmel_isi {
struct vb2_alloc_ctx *alloc_ctx;
/* Allocate descriptors for dma buffer use */
- struct fbd *p_fb_descriptors;
+ union fbd *p_fb_descriptors;
dma_addr_t fb_descriptors_phys;
struct list_head dma_desc_head;
struct isi_dma_desc dma_desc[MAX_BUFFER_NUM];
@@ -396,6 +400,16 @@ static int buffer_init(struct vb2_buffer *vb)
return 0;
}
+static void isi_hw_init_dma_desc(union fbd *p_fdb, u32 fb_addr,
+ u32 next_fbd_addr)
+{
+ struct fbd_isi_v2 *p = &(p_fdb->fbd_isi);
+
+ p->fb_address = fb_addr;
+ p->next_fbd_address = next_fbd_addr;
+ p->dma_ctrl = ISI_DMA_CTRL_WB;
+}
+
static int buffer_prepare(struct vb2_buffer *vb)
{
struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
@@ -428,10 +442,7 @@ static int buffer_prepare(struct vb2_buffer *vb)
list_del_init(&desc->list);
/* Initialize the dma descriptor */
- desc->p_fbd->fb_address =
- vb2_dma_contig_plane_dma_addr(vb, 0);
- desc->p_fbd->next_fbd_address = 0;
- desc->p_fbd->dma_ctrl = ISI_DMA_CTRL_WB;
+ isi_hw_init_dma_desc(desc->p_fbd, vb2_dma_contig_plane_dma_addr(vb, 0), 0);
buf->p_dma_desc = desc;
}
@@ -923,7 +934,7 @@ static int atmel_isi_remove(struct platform_device *pdev)
soc_camera_host_unregister(soc_host);
vb2_dma_contig_cleanup_ctx(isi->alloc_ctx);
dma_free_coherent(&pdev->dev,
- sizeof(struct fbd) * MAX_BUFFER_NUM,
+ sizeof(union fbd) * MAX_BUFFER_NUM,
isi->p_fb_descriptors,
isi->fb_descriptors_phys);
pm_runtime_disable(&pdev->dev);
@@ -1010,7 +1021,7 @@ static int atmel_isi_probe(struct platform_device *pdev)
INIT_LIST_HEAD(&isi->dma_desc_head);
isi->p_fb_descriptors = dma_alloc_coherent(&pdev->dev,
- sizeof(struct fbd) * MAX_BUFFER_NUM,
+ sizeof(union fbd) * MAX_BUFFER_NUM,
&isi->fb_descriptors_phys,
GFP_KERNEL);
if (!isi->p_fb_descriptors) {
@@ -1021,7 +1032,7 @@ static int atmel_isi_probe(struct platform_device *pdev)
for (i = 0; i < MAX_BUFFER_NUM; i++) {
isi->dma_desc[i].p_fbd = isi->p_fb_descriptors + i;
isi->dma_desc[i].fbd_phys = isi->fb_descriptors_phys +
- i * sizeof(struct fbd);
+ i * sizeof(union fbd);
list_add(&isi->dma_desc[i].list, &isi->dma_desc_head);
}
@@ -1080,7 +1091,7 @@ err_ioremap:
vb2_dma_contig_cleanup_ctx(isi->alloc_ctx);
err_alloc_ctx:
dma_free_coherent(&pdev->dev,
- sizeof(struct fbd) * MAX_BUFFER_NUM,
+ sizeof(union fbd) * MAX_BUFFER_NUM,
isi->p_fb_descriptors,
isi->fb_descriptors_phys);
--
1.9.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH 12/13] atmel-isi: use union for the fbd (frame buffer descriptor)
2016-01-18 12:52 ` [PATCH 12/13] atmel-isi: use union for the fbd (frame buffer descriptor) Josh Wu
@ 2016-01-24 19:31 ` Guennadi Liakhovetski
2016-01-26 14:04 ` Josh Wu
0 siblings, 1 reply; 29+ messages in thread
From: Guennadi Liakhovetski @ 2016-01-24 19:31 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, 18 Jan 2016, Josh Wu wrote:
> From: Josh Wu <josh.wu@atmel.com>
>
> This way, we can easy to add other type of fbd for new hardware.
Ok, I've applied all your 13 patches to check, what the resulting driver
would look like. To me it looks like you really abstract away _everything_
remotely hardware-specific. What is left is yet another abstraction layer,
into which you can pack a wide range of hardware types, which are very
different from the original ISI. I mean, you could probably pack - to some
extent, maybe sacrificing some features - other existing soc-camera
drivers, like MX3, MX2, CEU,... - essentially those, using VB2. And I
don't think that's a good idea. We have a class of V4L2 camera bridge
drivers, that's fine. They use all the standard APIs to connect to the
user-space and to other V4L2 drivers in video pipelines - V4L2 ioctl()s,
subdev, Media Controller, VB2, V4L2 control API etc. Under that we have
soc-camera - mainly for a few existing bridge drivers, because it takes a
part of bridge driver's implementation freedom away and many or most
modern camera bridge interfaces are more complex, than what soc-camera
currently supports, and extending it makes little sense, it is just more
logical to create a full-features V4L2 bridge driver with a full access to
all relevant APIs. With your patches #12 and #13 you seem to be creating
an even tighter, narrower API for very thin drivers. That just provide a
couple of hardware-related functions and create a V4L2 bridge driver from
that. What kind of hardware is that new controller, that you'd like to
support by the same driver? Wouldn't it be better to create a new driver
for it? Is it really similar to the ISI controller?
Thanks
Guennadi
>
> Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
> ---
>
> drivers/media/platform/soc_camera/atmel-isi.c | 33 ++++++++++++++++++---------
> 1 file changed, 22 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
> index 7d2e952..b4c1f38 100644
> --- a/drivers/media/platform/soc_camera/atmel-isi.c
> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> @@ -37,7 +37,7 @@
> #define FRAME_INTERVAL_MILLI_SEC (1000 / MIN_FRAME_RATE)
>
> /* Frame buffer descriptor */
> -struct fbd {
> +struct fbd_isi_v2 {
> /* Physical address of the frame buffer */
> u32 fb_address;
> /* DMA Control Register(only in HISI2) */
> @@ -46,9 +46,13 @@ struct fbd {
> u32 next_fbd_address;
> };
>
> +union fbd {
> + struct fbd_isi_v2 fbd_isi;
> +};
> +
> struct isi_dma_desc {
> struct list_head list;
> - struct fbd *p_fbd;
> + union fbd *p_fbd;
> dma_addr_t fbd_phys;
> };
>
> @@ -69,7 +73,7 @@ struct atmel_isi {
> struct vb2_alloc_ctx *alloc_ctx;
>
> /* Allocate descriptors for dma buffer use */
> - struct fbd *p_fb_descriptors;
> + union fbd *p_fb_descriptors;
> dma_addr_t fb_descriptors_phys;
> struct list_head dma_desc_head;
> struct isi_dma_desc dma_desc[MAX_BUFFER_NUM];
> @@ -396,6 +400,16 @@ static int buffer_init(struct vb2_buffer *vb)
> return 0;
> }
>
> +static void isi_hw_init_dma_desc(union fbd *p_fdb, u32 fb_addr,
> + u32 next_fbd_addr)
> +{
> + struct fbd_isi_v2 *p = &(p_fdb->fbd_isi);
Superfluous parentheses
> +
> + p->fb_address = fb_addr;
> + p->next_fbd_address = next_fbd_addr;
> + p->dma_ctrl = ISI_DMA_CTRL_WB;
> +}
> +
> static int buffer_prepare(struct vb2_buffer *vb)
> {
> struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> @@ -428,10 +442,7 @@ static int buffer_prepare(struct vb2_buffer *vb)
> list_del_init(&desc->list);
>
> /* Initialize the dma descriptor */
> - desc->p_fbd->fb_address =
> - vb2_dma_contig_plane_dma_addr(vb, 0);
> - desc->p_fbd->next_fbd_address = 0;
> - desc->p_fbd->dma_ctrl = ISI_DMA_CTRL_WB;
> + isi_hw_init_dma_desc(desc->p_fbd, vb2_dma_contig_plane_dma_addr(vb, 0), 0);
>
> buf->p_dma_desc = desc;
> }
> @@ -923,7 +934,7 @@ static int atmel_isi_remove(struct platform_device *pdev)
> soc_camera_host_unregister(soc_host);
> vb2_dma_contig_cleanup_ctx(isi->alloc_ctx);
> dma_free_coherent(&pdev->dev,
> - sizeof(struct fbd) * MAX_BUFFER_NUM,
> + sizeof(union fbd) * MAX_BUFFER_NUM,
> isi->p_fb_descriptors,
> isi->fb_descriptors_phys);
> pm_runtime_disable(&pdev->dev);
> @@ -1010,7 +1021,7 @@ static int atmel_isi_probe(struct platform_device *pdev)
> INIT_LIST_HEAD(&isi->dma_desc_head);
>
> isi->p_fb_descriptors = dma_alloc_coherent(&pdev->dev,
> - sizeof(struct fbd) * MAX_BUFFER_NUM,
> + sizeof(union fbd) * MAX_BUFFER_NUM,
> &isi->fb_descriptors_phys,
> GFP_KERNEL);
> if (!isi->p_fb_descriptors) {
> @@ -1021,7 +1032,7 @@ static int atmel_isi_probe(struct platform_device *pdev)
> for (i = 0; i < MAX_BUFFER_NUM; i++) {
> isi->dma_desc[i].p_fbd = isi->p_fb_descriptors + i;
> isi->dma_desc[i].fbd_phys = isi->fb_descriptors_phys +
> - i * sizeof(struct fbd);
> + i * sizeof(union fbd);
> list_add(&isi->dma_desc[i].list, &isi->dma_desc_head);
> }
>
> @@ -1080,7 +1091,7 @@ err_ioremap:
> vb2_dma_contig_cleanup_ctx(isi->alloc_ctx);
> err_alloc_ctx:
> dma_free_coherent(&pdev->dev,
> - sizeof(struct fbd) * MAX_BUFFER_NUM,
> + sizeof(union fbd) * MAX_BUFFER_NUM,
> isi->p_fb_descriptors,
> isi->fb_descriptors_phys);
>
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 29+ messages in thread* [PATCH 12/13] atmel-isi: use union for the fbd (frame buffer descriptor)
2016-01-24 19:31 ` Guennadi Liakhovetski
@ 2016-01-26 14:04 ` Josh Wu
2016-01-26 14:10 ` Guennadi Liakhovetski
0 siblings, 1 reply; 29+ messages in thread
From: Josh Wu @ 2016-01-26 14:04 UTC (permalink / raw)
To: linux-arm-kernel
add all people in reply,
Hi, Guennadi
Thanks for the review.
2016-01-25 3:31 GMT+08:00 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> On Mon, 18 Jan 2016, Josh Wu wrote:
>
>> From: Josh Wu <josh.wu@atmel.com>
>>
>> This way, we can easy to add other type of fbd for new hardware.
>
> Ok, I've applied all your 13 patches to check, what the resulting driver
> would look like. To me it looks like you really abstract away _everything_
> remotely hardware-specific. What is left is yet another abstraction layer,
> into which you can pack a wide range of hardware types, which are very
> different from the original ISI. I mean, you could probably pack - to some
> extent, maybe sacrificing some features - other existing soc-camera
> drivers, like MX3, MX2, CEU,... - essentially those, using VB2. And I
> don't think that's a good idea. We have a class of V4L2 camera bridge
> drivers, that's fine. They use all the standard APIs to connect to the
> user-space and to other V4L2 drivers in video pipelines - V4L2 ioctl()s,
> subdev, Media Controller, VB2, V4L2 control API etc. Under that we have
> soc-camera - mainly for a few existing bridge drivers, because it takes a
> part of bridge driver's implementation freedom away and many or most
> modern camera bridge interfaces are more complex, than what soc-camera
> currently supports, and extending it makes little sense, it is just more
> logical to create a full-features V4L2 bridge driver with a full access to
> all relevant APIs.
It sounds the general v4l2 driver framework is more suitable than
soc-camera framework for the new hardware.
So is it easy for v4l2 platform driver to use the soc-camera sensors?
> With your patches #12 and #13 you seem to be creating
> an even tighter, narrower API for very thin drivers. That just provide a
> couple of hardware-related functions and create a V4L2 bridge driver from
> that. What kind of hardware is that new controller, that you'd like to
> support by the same driver? Wouldn't it be better to create a new driver
> for it? Is it really similar to the ISI controller?
The new hardware is SAMA5D2 Image Sensor Controller. You can find the
datasheet here:
http://www.atmel.com/Images/Atmel-11267-32-bit-Cortex-A5-Microcontroller-SAMA5D2_Datasheet.pdf
Actually, The ISC hardware is very different from ISI hardware. ISC
has no Preivew/Codec path, it just has many data blocks to process
sensor data.
With the abstraction of my patches, ISC can rewrite the interrupt
handler, initialization, configure and etc to work in same ISI driver,
though. But like you mentioned, it's very tight, maybe it's not easy
to add extend functions.
So I was convinced to write a new v4l2 camera driver for ISC if it is
easy to support soc-camera sensors.
>
> Thanks
> Guennadi
Best Regards,
Josh Wu
>
>>
>> Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
>> ---
>>
>> drivers/media/platform/soc_camera/atmel-isi.c | 33 ++++++++++++++++++---------
>> 1 file changed, 22 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
>> index 7d2e952..b4c1f38 100644
>> --- a/drivers/media/platform/soc_camera/atmel-isi.c
>> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
>> @@ -37,7 +37,7 @@
>> #define FRAME_INTERVAL_MILLI_SEC (1000 / MIN_FRAME_RATE)
>>
>> /* Frame buffer descriptor */
>> -struct fbd {
>> +struct fbd_isi_v2 {
>> /* Physical address of the frame buffer */
>> u32 fb_address;
>> /* DMA Control Register(only in HISI2) */
>> @@ -46,9 +46,13 @@ struct fbd {
>> u32 next_fbd_address;
>> };
>>
>> +union fbd {
>> + struct fbd_isi_v2 fbd_isi;
>> +};
>> +
>> struct isi_dma_desc {
>> struct list_head list;
>> - struct fbd *p_fbd;
>> + union fbd *p_fbd;
>> dma_addr_t fbd_phys;
>> };
>>
>> @@ -69,7 +73,7 @@ struct atmel_isi {
>> struct vb2_alloc_ctx *alloc_ctx;
>>
>> /* Allocate descriptors for dma buffer use */
>> - struct fbd *p_fb_descriptors;
>> + union fbd *p_fb_descriptors;
>> dma_addr_t fb_descriptors_phys;
>> struct list_head dma_desc_head;
>> struct isi_dma_desc dma_desc[MAX_BUFFER_NUM];
>> @@ -396,6 +400,16 @@ static int buffer_init(struct vb2_buffer *vb)
>> return 0;
>> }
>>
>> +static void isi_hw_init_dma_desc(union fbd *p_fdb, u32 fb_addr,
>> + u32 next_fbd_addr)
>> +{
>> + struct fbd_isi_v2 *p = &(p_fdb->fbd_isi);
>
> Superfluous parentheses
>
>> +
>> + p->fb_address = fb_addr;
>> + p->next_fbd_address = next_fbd_addr;
>> + p->dma_ctrl = ISI_DMA_CTRL_WB;
>> +}
>> +
>> static int buffer_prepare(struct vb2_buffer *vb)
>> {
>> struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>> @@ -428,10 +442,7 @@ static int buffer_prepare(struct vb2_buffer *vb)
>> list_del_init(&desc->list);
>>
>> /* Initialize the dma descriptor */
>> - desc->p_fbd->fb_address =
>> - vb2_dma_contig_plane_dma_addr(vb, 0);
>> - desc->p_fbd->next_fbd_address = 0;
>> - desc->p_fbd->dma_ctrl = ISI_DMA_CTRL_WB;
>> + isi_hw_init_dma_desc(desc->p_fbd, vb2_dma_contig_plane_dma_addr(vb, 0), 0);
>>
>> buf->p_dma_desc = desc;
>> }
>> @@ -923,7 +934,7 @@ static int atmel_isi_remove(struct platform_device *pdev)
>> soc_camera_host_unregister(soc_host);
>> vb2_dma_contig_cleanup_ctx(isi->alloc_ctx);
>> dma_free_coherent(&pdev->dev,
>> - sizeof(struct fbd) * MAX_BUFFER_NUM,
>> + sizeof(union fbd) * MAX_BUFFER_NUM,
>> isi->p_fb_descriptors,
>> isi->fb_descriptors_phys);
>> pm_runtime_disable(&pdev->dev);
>> @@ -1010,7 +1021,7 @@ static int atmel_isi_probe(struct platform_device *pdev)
>> INIT_LIST_HEAD(&isi->dma_desc_head);
>>
>> isi->p_fb_descriptors = dma_alloc_coherent(&pdev->dev,
>> - sizeof(struct fbd) * MAX_BUFFER_NUM,
>> + sizeof(union fbd) * MAX_BUFFER_NUM,
>> &isi->fb_descriptors_phys,
>> GFP_KERNEL);
>> if (!isi->p_fb_descriptors) {
>> @@ -1021,7 +1032,7 @@ static int atmel_isi_probe(struct platform_device *pdev)
>> for (i = 0; i < MAX_BUFFER_NUM; i++) {
>> isi->dma_desc[i].p_fbd = isi->p_fb_descriptors + i;
>> isi->dma_desc[i].fbd_phys = isi->fb_descriptors_phys +
>> - i * sizeof(struct fbd);
>> + i * sizeof(union fbd);
>> list_add(&isi->dma_desc[i].list, &isi->dma_desc_head);
>> }
>>
>> @@ -1080,7 +1091,7 @@ err_ioremap:
>> vb2_dma_contig_cleanup_ctx(isi->alloc_ctx);
>> err_alloc_ctx:
>> dma_free_coherent(&pdev->dev,
>> - sizeof(struct fbd) * MAX_BUFFER_NUM,
>> + sizeof(union fbd) * MAX_BUFFER_NUM,
>> isi->p_fb_descriptors,
>> isi->fb_descriptors_phys);
>>
>> --
>> 1.9.1
>>
^ permalink raw reply [flat|nested] 29+ messages in thread* [PATCH 12/13] atmel-isi: use union for the fbd (frame buffer descriptor)
2016-01-26 14:04 ` Josh Wu
@ 2016-01-26 14:10 ` Guennadi Liakhovetski
2016-01-26 14:24 ` Josh Wu
0 siblings, 1 reply; 29+ messages in thread
From: Guennadi Liakhovetski @ 2016-01-26 14:10 UTC (permalink / raw)
To: linux-arm-kernel
Hi Josh,
(resending with all CC)
On Tue, 26 Jan 2016, Josh Wu wrote:
> Hi, Guennadi
>
> Thanks for the review.
>
> 2016-01-25 3:31 GMT+08:00 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> > On Mon, 18 Jan 2016, Josh Wu wrote:
> >
> >> From: Josh Wu <josh.wu@atmel.com>
> >>
> >> This way, we can easy to add other type of fbd for new hardware.
> >
> > Ok, I've applied all your 13 patches to check, what the resulting driver
> > would look like. To me it looks like you really abstract away _everything_
> > remotely hardware-specific. What is left is yet another abstraction layer,
> > into which you can pack a wide range of hardware types, which are very
> > different from the original ISI. I mean, you could probably pack - to some
> > extent, maybe sacrificing some features - other existing soc-camera
> > drivers, like MX3, MX2, CEU,... - essentially those, using VB2. And I
> > don't think that's a good idea. We have a class of V4L2 camera bridge
> > drivers, that's fine. They use all the standard APIs to connect to the
> > user-space and to other V4L2 drivers in video pipelines - V4L2 ioctl()s,
> > subdev, Media Controller, VB2, V4L2 control API etc. Under that we have
> > soc-camera - mainly for a few existing bridge drivers, because it takes a
> > part of bridge driver's implementation freedom away and many or most
> > modern camera bridge interfaces are more complex, than what soc-camera
> > currently supports, and extending it makes little sense, it is just more
> > logical to create a full-features V4L2 bridge driver with a full access to
> > all relevant APIs.
>
> It sounds the general v4l2 driver framework is more suitable than
> soc-camera framework for the new hardware.
Then, please, go for one!
> So is it easy for v4l2 platform driver to use the soc-camera sensors?
Not sure, haven't tried in a while. It used to be difficult, but it must
have become more simple, I think there are examples of that in the
mainline. I think em28xx does that, but probably in the meantime the
integration possibilities have become even better.
> > With your patches #12 and #13 you seem to be creating
> > an even tighter, narrower API for very thin drivers. That just provide a
> > couple of hardware-related functions and create a V4L2 bridge driver from
> > that. What kind of hardware is that new controller, that you'd like to
> > support by the same driver? Wouldn't it be better to create a new driver
> > for it? Is it really similar to the ISI controller?
>
> The new hardware is SAMA5D2 Image Sensor Controller. You can find the
> datasheet here:
> http://www.atmel.com/Images/Atmel-11267-32-bit-Cortex-A5-Microcontroller-SAMA5D2_Datasheet.pdf
>
> Actually, The ISC hardware is very different from ISI hardware. ISC
> has no Preivew/Codec path, it just has many data blocks to process
> sensor data.
> With the abstraction of my patches, ISC can rewrite the interrupt
> handler, initialization, configure and etc to work in same ISI driver,
> though. But like you mentioned, it's very tight, maybe it's not easy
> to add extend functions.
>
> So I was convinced to write a new v4l2 camera driver for ISC if it is
> easy to support soc-camera sensors.
Please, write a new driver :)
Thanks
Guennadi
> Best Regards,
> Josh Wu
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 12/13] atmel-isi: use union for the fbd (frame buffer descriptor)
2016-01-26 14:10 ` Guennadi Liakhovetski
@ 2016-01-26 14:24 ` Josh Wu
2016-01-26 14:39 ` Guennadi Liakhovetski
0 siblings, 1 reply; 29+ messages in thread
From: Josh Wu @ 2016-01-26 14:24 UTC (permalink / raw)
To: linux-arm-kernel
Hi, Guennadi
2016-01-26 22:10 GMT+08:00 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> Hi Josh,
>
> (resending with all CC)
>
> On Tue, 26 Jan 2016, Josh Wu wrote:
>
>> Hi, Guennadi
>>
>> Thanks for the review.
>>
>> 2016-01-25 3:31 GMT+08:00 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
>> > On Mon, 18 Jan 2016, Josh Wu wrote:
>> >
>> >> From: Josh Wu <josh.wu@atmel.com>
>> >>
>> >> This way, we can easy to add other type of fbd for new hardware.
>> >
>> > Ok, I've applied all your 13 patches to check, what the resulting driver
>> > would look like. To me it looks like you really abstract away _everything_
>> > remotely hardware-specific. What is left is yet another abstraction layer,
>> > into which you can pack a wide range of hardware types, which are very
>> > different from the original ISI. I mean, you could probably pack - to some
>> > extent, maybe sacrificing some features - other existing soc-camera
>> > drivers, like MX3, MX2, CEU,... - essentially those, using VB2. And I
>> > don't think that's a good idea. We have a class of V4L2 camera bridge
>> > drivers, that's fine. They use all the standard APIs to connect to the
>> > user-space and to other V4L2 drivers in video pipelines - V4L2 ioctl()s,
>> > subdev, Media Controller, VB2, V4L2 control API etc. Under that we have
>> > soc-camera - mainly for a few existing bridge drivers, because it takes a
>> > part of bridge driver's implementation freedom away and many or most
>> > modern camera bridge interfaces are more complex, than what soc-camera
>> > currently supports, and extending it makes little sense, it is just more
>> > logical to create a full-features V4L2 bridge driver with a full access to
>> > all relevant APIs.
>>
>> It sounds the general v4l2 driver framework is more suitable than
>> soc-camera framework for the new hardware.
>
> Then, please, go for one!
>
>> So is it easy for v4l2 platform driver to use the soc-camera sensors?
>
> Not sure, haven't tried in a while. It used to be difficult, but it must
> have become more simple, I think there are examples of that in the
> mainline. I think em28xx does that, but probably in the meantime the
> integration possibilities have become even better.
I'll try to see what I'll get.
>
>> > With your patches #12 and #13 you seem to be creating
>> > an even tighter, narrower API for very thin drivers. That just provide a
>> > couple of hardware-related functions and create a V4L2 bridge driver from
>> > that. What kind of hardware is that new controller, that you'd like to
>> > support by the same driver? Wouldn't it be better to create a new driver
>> > for it? Is it really similar to the ISI controller?
>>
>> The new hardware is SAMA5D2 Image Sensor Controller. You can find the
>> datasheet here:
>> http://www.atmel.com/Images/Atmel-11267-32-bit-Cortex-A5-Microcontroller-SAMA5D2_Datasheet.pdf
>>
>> Actually, The ISC hardware is very different from ISI hardware. ISC
>> has no Preivew/Codec path, it just has many data blocks to process
>> sensor data.
>> With the abstraction of my patches, ISC can rewrite the interrupt
>> handler, initialization, configure and etc to work in same ISI driver,
>> though. But like you mentioned, it's very tight, maybe it's not easy
>> to add extend functions.
>>
>> So I was convinced to write a new v4l2 camera driver for ISC if it is
>> easy to support soc-camera sensors.
>
> Please, write a new driver :)
Ok, so for this serial patch, I will drop the abstract interface
relevant patches, and keep the refactor patch in v2. Thanks.
Best Regards,
Josh Wu
>
> Thanks
> Guennadi
>
>> Best Regards,
>> Josh Wu
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 12/13] atmel-isi: use union for the fbd (frame buffer descriptor)
2016-01-26 14:24 ` Josh Wu
@ 2016-01-26 14:39 ` Guennadi Liakhovetski
0 siblings, 0 replies; 29+ messages in thread
From: Guennadi Liakhovetski @ 2016-01-26 14:39 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, 26 Jan 2016, Josh Wu wrote:
> Ok, so for this serial patch, I will drop the abstract interface
> relevant patches, and keep the refactor patch in v2. Thanks.
Yes, please, drop the last 2 patches, and, preferably, the "polling
instead of IRQ" one too.
Thanks
Guennadi
^ permalink raw reply [flat|nested] 29+ messages in thread
* [PATCH 13/13] atmel-isi: use an hw_data structure according compatible string
2016-01-18 12:52 ` [PATCH 06/13] atmel-isi: check ISI_SR's flags by polling instead of interrupt Josh Wu
` (6 preceding siblings ...)
2016-01-18 12:52 ` [PATCH 12/13] atmel-isi: use union for the fbd (frame buffer descriptor) Josh Wu
@ 2016-01-18 12:52 ` Josh Wu
2016-01-24 16:58 ` [PATCH 06/13] atmel-isi: check ISI_SR's flags by polling instead of interrupt Guennadi Liakhovetski
8 siblings, 0 replies; 29+ messages in thread
From: Josh Wu @ 2016-01-18 12:52 UTC (permalink / raw)
To: linux-arm-kernel
From: Josh Wu <josh.wu@atmel.com>
The hw_data can define new hw_ops, hw_regs, which has all hardware related
data. That can make us easy to add another hardware support.
Also rename the hw related function with 'isi_' prefix.
Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
---
drivers/media/platform/soc_camera/atmel-isi.c | 88 ++++++++++++++++++++++-----
1 file changed, 72 insertions(+), 16 deletions(-)
diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
index b4c1f38..7f7be7d 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -18,6 +18,7 @@
#include <linux/interrupt.h>
#include <linux/kernel.h>
#include <linux/module.h>
+#include <linux/of_device.h>
#include <linux/platform_device.h>
#include <linux/pm_runtime.h>
#include <linux/slab.h>
@@ -91,6 +92,8 @@ struct atmel_isi {
struct frame_buffer *active;
struct soc_camera_host soc_host;
+ struct at91_camera_hw_ops *hw_ops;
+ struct at91_camera_hw_regs *hw_regs;
};
static void isi_writel(struct atmel_isi *isi, u32 reg, u32 val)
@@ -102,6 +105,29 @@ static u32 isi_readl(struct atmel_isi *isi, u32 reg)
return readl(isi->regs + reg);
}
+struct at91_camera_hw_ops {
+ /* start dma transfer */
+ void (*hw_start_dma)(struct atmel_isi *isi, struct frame_buffer *buf);
+ /* start ISI hardware capture */
+ void (*hw_start)(struct atmel_isi *isi);
+ int (*hw_initialize)(struct atmel_isi *isi);
+ void (*hw_uninitialize)(struct atmel_isi *isi);
+ void (*hw_configure)(struct atmel_isi *isi, u32 width, u32 height,
+ const struct soc_camera_format_xlate *xlate);
+ irqreturn_t (*hw_interrupt)(int irq, void *dev_id);
+ void (*hw_init_dma_desc)(union fbd *p_fdb, u32 fb_addr,
+ u32 next_fbd_addr);
+};
+
+struct at91_camera_hw_regs {
+ u32 status_reg_offset;
+};
+
+struct at91_camera_hw_data {
+ struct at91_camera_hw_ops *hw_ops;
+ struct at91_camera_hw_regs *hw_regs;
+};
+
static u32 setup_cfg2_yuv_swap(struct atmel_isi *isi,
const struct soc_camera_format_xlate *xlate)
{
@@ -140,7 +166,7 @@ static u32 setup_cfg2_yuv_swap(struct atmel_isi *isi,
return ISI_CFG2_YCC_SWAP_DEFAULT;
}
-static void configure_geometry(struct atmel_isi *isi, u32 width,
+static void isi_configure_geometry(struct atmel_isi *isi, u32 width,
u32 height, const struct soc_camera_format_xlate *xlate)
{
u32 cfg2, psize;
@@ -192,8 +218,8 @@ static int isi_hw_wait_status(struct atmel_isi *isi, int status_flag,
{
unsigned long timeout = jiffies + wait_ms * HZ;
- while ((isi_readl(isi, ISI_STATUS) & status_flag) &&
- time_before(jiffies, timeout))
+ while ((isi_readl(isi, isi->hw_regs->status_reg_offset) & status_flag)
+ && time_before(jiffies, timeout))
msleep(1);
if (time_after(jiffies, timeout))
@@ -274,7 +300,7 @@ static void isi_hw_uninitialize(struct atmel_isi *isi)
dev_err(isi->soc_host.icd->parent, "Disable ISI timed out\n");
}
-static void start_isi(struct atmel_isi *isi)
+static void isi_start(struct atmel_isi *isi)
{
u32 ctrl;
@@ -288,7 +314,7 @@ static void start_isi(struct atmel_isi *isi)
isi_writel(isi, ISI_CTRL, ctrl);
}
-static void start_dma(struct atmel_isi *isi, struct frame_buffer *buffer)
+static void isi_start_dma(struct atmel_isi *isi, struct frame_buffer *buffer)
{
/* Check if already in a frame */
if (!isi->enable_preview_path) {
@@ -330,7 +356,8 @@ static irqreturn_t atmel_isi_handle_streaming(struct atmel_isi *isi)
/* start next dma frame. */
isi->active = list_entry(isi->video_buffer_list.next,
struct frame_buffer, list);
- start_dma(isi, isi->active);
+
+ (*isi->hw_ops->hw_start_dma)(isi, isi->active);
}
return IRQ_HANDLED;
}
@@ -419,6 +446,7 @@ static int buffer_prepare(struct vb2_buffer *vb)
struct atmel_isi *isi = ici->priv;
unsigned long size;
struct isi_dma_desc *desc;
+ u32 vb_addr;
size = icd->sizeimage;
@@ -442,7 +470,8 @@ static int buffer_prepare(struct vb2_buffer *vb)
list_del_init(&desc->list);
/* Initialize the dma descriptor */
- isi_hw_init_dma_desc(desc->p_fbd, vb2_dma_contig_plane_dma_addr(vb, 0), 0);
+ vb_addr = vb2_dma_contig_plane_dma_addr(vb, 0);
+ (*isi->hw_ops->hw_init_dma_desc)(desc->p_fbd, vb_addr, 0);
buf->p_dma_desc = desc;
}
@@ -478,7 +507,7 @@ static void buffer_queue(struct vb2_buffer *vb)
if (isi->active == NULL) {
isi->active = buf;
if (vb2_is_streaming(vb->vb2_queue))
- start_dma(isi, buf);
+ (*isi->hw_ops->hw_start_dma)(isi, buf);
}
spin_unlock_irqrestore(&isi->lock, flags);
}
@@ -492,20 +521,20 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
pm_runtime_get_sync(ici->v4l2_dev.dev);
- ret = isi_hw_initialize(isi);
+ ret = (*isi->hw_ops->hw_initialize)(isi);
if (ret) {
pm_runtime_put(ici->v4l2_dev.dev);
return ret;
}
- configure_geometry(isi, icd->user_width, icd->user_height,
- icd->current_fmt);
+ (*isi->hw_ops->hw_configure)(isi, icd->user_width, icd->user_height,
+ icd->current_fmt);
spin_lock_irq(&isi->lock);
if (count) {
- start_dma(isi, isi->active);
- start_isi(isi);
+ (*isi->hw_ops->hw_start_dma)(isi, isi->active);
+ (*isi->hw_ops->hw_start)(isi);
}
spin_unlock_irq(&isi->lock);
@@ -530,7 +559,7 @@ static void stop_streaming(struct vb2_queue *vq)
}
spin_unlock_irq(&isi->lock);
- isi_hw_uninitialize(isi);
+ (*isi->hw_ops->hw_uninitialize)(isi);
pm_runtime_put(ici->v4l2_dev.dev);
}
@@ -993,6 +1022,7 @@ static int atmel_isi_parse_dt(struct atmel_isi *isi,
return 0;
}
+static const struct of_device_id atmel_isi_of_match[];
static int atmel_isi_probe(struct platform_device *pdev)
{
unsigned int irq;
@@ -1000,6 +1030,7 @@ static int atmel_isi_probe(struct platform_device *pdev)
struct resource *regs;
int ret, i;
struct soc_camera_host *soc_host;
+ struct at91_camera_hw_data *hw_data;
isi = devm_kzalloc(&pdev->dev, sizeof(struct atmel_isi), GFP_KERNEL);
if (!isi) {
@@ -1015,6 +1046,11 @@ static int atmel_isi_probe(struct platform_device *pdev)
if (ret)
return ret;
+ hw_data = (struct at91_camera_hw_data *)
+ of_match_device(atmel_isi_of_match, &pdev->dev)->data;
+ isi->hw_ops = hw_data->hw_ops;
+ isi->hw_regs = hw_data->hw_regs;
+
isi->active = NULL;
spin_lock_init(&isi->lock);
INIT_LIST_HEAD(&isi->video_buffer_list);
@@ -1060,7 +1096,8 @@ static int atmel_isi_probe(struct platform_device *pdev)
goto err_req_irq;
}
- ret = devm_request_irq(&pdev->dev, irq, isi_interrupt, 0, "isi", isi);
+ ret = devm_request_irq(&pdev->dev, irq, isi->hw_ops->hw_interrupt, 0,
+ "isi", isi);
if (ret) {
dev_err(&pdev->dev, "Unable to request irq %d\n", irq);
goto err_req_irq;
@@ -1119,13 +1156,32 @@ static int atmel_isi_runtime_resume(struct device *dev)
}
#endif /* CONFIG_PM */
+static struct at91_camera_hw_ops at91sam9g45_ops = {
+ .hw_initialize = isi_hw_initialize,
+ .hw_uninitialize = isi_hw_uninitialize,
+ .hw_configure = isi_configure_geometry,
+ .hw_start = isi_start,
+ .hw_start_dma = isi_start_dma,
+ .hw_interrupt = isi_interrupt,
+ .hw_init_dma_desc = isi_hw_init_dma_desc,
+};
+
+static struct at91_camera_hw_regs at91sam9g45_regs = {
+ .status_reg_offset = ISI_STATUS,
+};
+
+static struct at91_camera_hw_data at91sam9g45_data = {
+ .hw_ops = &at91sam9g45_ops,
+ .hw_regs = &at91sam9g45_regs,
+};
+
static const struct dev_pm_ops atmel_isi_dev_pm_ops = {
SET_RUNTIME_PM_OPS(atmel_isi_runtime_suspend,
atmel_isi_runtime_resume, NULL)
};
static const struct of_device_id atmel_isi_of_match[] = {
- { .compatible = "atmel,at91sam9g45-isi" },
+ { .compatible = "atmel,at91sam9g45-isi", .data = &at91sam9g45_data },
{ }
};
MODULE_DEVICE_TABLE(of, atmel_isi_of_match);
--
1.9.1
^ permalink raw reply related [flat|nested] 29+ messages in thread* [PATCH 06/13] atmel-isi: check ISI_SR's flags by polling instead of interrupt
2016-01-18 12:52 ` [PATCH 06/13] atmel-isi: check ISI_SR's flags by polling instead of interrupt Josh Wu
` (7 preceding siblings ...)
2016-01-18 12:52 ` [PATCH 13/13] atmel-isi: use an hw_data structure according compatible string Josh Wu
@ 2016-01-24 16:58 ` Guennadi Liakhovetski
2016-01-26 14:16 ` Josh Wu
8 siblings, 1 reply; 29+ messages in thread
From: Guennadi Liakhovetski @ 2016-01-24 16:58 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, 18 Jan 2016, Josh Wu wrote:
> In current code, we use a interrupt to check whether ISI reset/disable
> action is done. Actually, we also can check ISI SR to check the
> reset/disable action by polling, and it is simpler and straight forward.
>
> So this patch use isi_hw_wait_status() to check the action status. As
> the interrupt checking the status is useless, so just remove the interrupt
> & completion code.
Sorry, I'm not convinced. Switching from interrupt-driven to polling seems
counter-productive to me. Why do you want this?
Thanks
Guennadi
>
> Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
> ---
>
> drivers/media/platform/soc_camera/atmel-isi.c | 59 ++++++---------------------
> 1 file changed, 13 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
> index f0508ea..4ddc309 100644
> --- a/drivers/media/platform/soc_camera/atmel-isi.c
> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> @@ -12,7 +12,6 @@
> */
>
> #include <linux/clk.h>
> -#include <linux/completion.h>
> #include <linux/delay.h>
> #include <linux/fs.h>
> #include <linux/init.h>
> @@ -81,7 +80,6 @@ struct atmel_isi {
> struct isi_dma_desc dma_desc[MAX_BUFFER_NUM];
> bool enable_preview_path;
>
> - struct completion complete;
> /* ISI peripherial clock */
> struct clk *pclk;
> unsigned int irq;
> @@ -281,51 +279,14 @@ static irqreturn_t isi_interrupt(int irq, void *dev_id)
> mask = isi_readl(isi, ISI_INTMASK);
> pending = status & mask;
>
> - 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 (likely(pending & ISI_SR_CXFR_DONE) ||
> - likely(pending & ISI_SR_PXFR_DONE))
> - ret = atmel_isi_handle_streaming(isi);
> - }
> + if (likely(pending & ISI_SR_CXFR_DONE) ||
> + likely(pending & ISI_SR_PXFR_DONE))
> + ret = atmel_isi_handle_streaming(isi);
>
> spin_unlock(&isi->lock);
> return ret;
> }
>
> -#define WAIT_ISI_RESET 1
> -#define WAIT_ISI_DISABLE 0
> -static int atmel_isi_wait_status(struct atmel_isi *isi, int wait_reset)
> -{
> - unsigned long timeout;
> - /*
> - * The reset or disable will only succeed if we have a
> - * pixel clock from the camera.
> - */
> - init_completion(&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->complete,
> - msecs_to_jiffies(500));
> - if (timeout == 0)
> - return -ETIMEDOUT;
> -
> - return 0;
> -}
> -
> /* ------------------------------------------------------------------
> Videobuf operations
> ------------------------------------------------------------------*/
> @@ -493,8 +454,11 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
> pm_runtime_get_sync(ici->v4l2_dev.dev);
>
> /* Reset ISI */
> - ret = atmel_isi_wait_status(isi, WAIT_ISI_RESET);
> - if (ret < 0) {
> + isi_writel(isi, ISI_CTRL, ISI_CTRL_SRST);
> +
> + /* Check Reset status */
> + ret = isi_hw_wait_status(isi, ISI_CTRL_SRST, 500);
> + if (ret) {
> dev_err(icd->parent, "Reset ISI timed out\n");
> pm_runtime_put(ici->v4l2_dev.dev);
> return ret;
> @@ -549,8 +513,11 @@ static void stop_streaming(struct vb2_queue *vq)
> ISI_SR_CXFR_DONE | ISI_SR_PXFR_DONE);
>
> /* Disable ISI and wait for it is done */
> - ret = atmel_isi_wait_status(isi, WAIT_ISI_DISABLE);
> - if (ret < 0)
> + isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
> +
> + /* Check Reset status */
> + ret = isi_hw_wait_status(isi, ISI_CTRL_DIS, 500);
> + if (ret)
> dev_err(icd->parent, "Disable ISI timed out\n");
>
> pm_runtime_put(ici->v4l2_dev.dev);
> --
> 1.9.1
>
^ permalink raw reply [flat|nested] 29+ messages in thread* [PATCH 06/13] atmel-isi: check ISI_SR's flags by polling instead of interrupt
2016-01-24 16:58 ` [PATCH 06/13] atmel-isi: check ISI_SR's flags by polling instead of interrupt Guennadi Liakhovetski
@ 2016-01-26 14:16 ` Josh Wu
2016-01-26 14:38 ` Guennadi Liakhovetski
0 siblings, 1 reply; 29+ messages in thread
From: Josh Wu @ 2016-01-26 14:16 UTC (permalink / raw)
To: linux-arm-kernel
Hi, Guennadi
2016-01-25 0:58 GMT+08:00 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> On Mon, 18 Jan 2016, Josh Wu wrote:
>
>> In current code, we use a interrupt to check whether ISI reset/disable
>> action is done. Actually, we also can check ISI SR to check the
>> reset/disable action by polling, and it is simpler and straight forward.
>>
>> So this patch use isi_hw_wait_status() to check the action status. As
>> the interrupt checking the status is useless, so just remove the interrupt
>> & completion code.
>
> Sorry, I'm not convinced. Switching from interrupt-driven to polling seems
> counter-productive to me. Why do you want this?
Yes, it is counter-productive If such code is called very frequently.
But here we only used for reset and disable the hardware.
I think it is more straightforward and simple when we just check
status register after write the reset or disable the ISI control
register. Using interrupt here is over designed.
for the sake of performance I should use cpu_releax() instead of
msleep() when waiting for the status bit.
Best Regards,
Josh Wu
>
> Thanks
> Guennadi
>
>>
>> Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
>> ---
>>
>> drivers/media/platform/soc_camera/atmel-isi.c | 59 ++++++---------------------
>> 1 file changed, 13 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
>> index f0508ea..4ddc309 100644
>> --- a/drivers/media/platform/soc_camera/atmel-isi.c
>> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
>> @@ -12,7 +12,6 @@
>> */
>>
>> #include <linux/clk.h>
>> -#include <linux/completion.h>
>> #include <linux/delay.h>
>> #include <linux/fs.h>
>> #include <linux/init.h>
>> @@ -81,7 +80,6 @@ struct atmel_isi {
>> struct isi_dma_desc dma_desc[MAX_BUFFER_NUM];
>> bool enable_preview_path;
>>
>> - struct completion complete;
>> /* ISI peripherial clock */
>> struct clk *pclk;
>> unsigned int irq;
>> @@ -281,51 +279,14 @@ static irqreturn_t isi_interrupt(int irq, void *dev_id)
>> mask = isi_readl(isi, ISI_INTMASK);
>> pending = status & mask;
>>
>> - 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 (likely(pending & ISI_SR_CXFR_DONE) ||
>> - likely(pending & ISI_SR_PXFR_DONE))
>> - ret = atmel_isi_handle_streaming(isi);
>> - }
>> + if (likely(pending & ISI_SR_CXFR_DONE) ||
>> + likely(pending & ISI_SR_PXFR_DONE))
>> + ret = atmel_isi_handle_streaming(isi);
>>
>> spin_unlock(&isi->lock);
>> return ret;
>> }
>>
>> -#define WAIT_ISI_RESET 1
>> -#define WAIT_ISI_DISABLE 0
>> -static int atmel_isi_wait_status(struct atmel_isi *isi, int wait_reset)
>> -{
>> - unsigned long timeout;
>> - /*
>> - * The reset or disable will only succeed if we have a
>> - * pixel clock from the camera.
>> - */
>> - init_completion(&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->complete,
>> - msecs_to_jiffies(500));
>> - if (timeout == 0)
>> - return -ETIMEDOUT;
>> -
>> - return 0;
>> -}
>> -
>> /* ------------------------------------------------------------------
>> Videobuf operations
>> ------------------------------------------------------------------*/
>> @@ -493,8 +454,11 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
>> pm_runtime_get_sync(ici->v4l2_dev.dev);
>>
>> /* Reset ISI */
>> - ret = atmel_isi_wait_status(isi, WAIT_ISI_RESET);
>> - if (ret < 0) {
>> + isi_writel(isi, ISI_CTRL, ISI_CTRL_SRST);
>> +
>> + /* Check Reset status */
>> + ret = isi_hw_wait_status(isi, ISI_CTRL_SRST, 500);
>> + if (ret) {
>> dev_err(icd->parent, "Reset ISI timed out\n");
>> pm_runtime_put(ici->v4l2_dev.dev);
>> return ret;
>> @@ -549,8 +513,11 @@ static void stop_streaming(struct vb2_queue *vq)
>> ISI_SR_CXFR_DONE | ISI_SR_PXFR_DONE);
>>
>> /* Disable ISI and wait for it is done */
>> - ret = atmel_isi_wait_status(isi, WAIT_ISI_DISABLE);
>> - if (ret < 0)
>> + isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
>> +
>> + /* Check Reset status */
>> + ret = isi_hw_wait_status(isi, ISI_CTRL_DIS, 500);
>> + if (ret)
>> dev_err(icd->parent, "Disable ISI timed out\n");
>>
>> pm_runtime_put(ici->v4l2_dev.dev);
>> --
>> 1.9.1
>>
^ permalink raw reply [flat|nested] 29+ messages in thread* [PATCH 06/13] atmel-isi: check ISI_SR's flags by polling instead of interrupt
2016-01-26 14:16 ` Josh Wu
@ 2016-01-26 14:38 ` Guennadi Liakhovetski
0 siblings, 0 replies; 29+ messages in thread
From: Guennadi Liakhovetski @ 2016-01-26 14:38 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, 26 Jan 2016, Josh Wu wrote:
> Hi, Guennadi
>
> 2016-01-25 0:58 GMT+08:00 Guennadi Liakhovetski <g.liakhovetski@gmx.de>:
> > On Mon, 18 Jan 2016, Josh Wu wrote:
> >
> >> In current code, we use a interrupt to check whether ISI reset/disable
> >> action is done. Actually, we also can check ISI SR to check the
> >> reset/disable action by polling, and it is simpler and straight forward.
> >>
> >> So this patch use isi_hw_wait_status() to check the action status. As
> >> the interrupt checking the status is useless, so just remove the interrupt
> >> & completion code.
> >
> > Sorry, I'm not convinced. Switching from interrupt-driven to polling seems
> > counter-productive to me. Why do you want this?
>
> Yes, it is counter-productive If such code is called very frequently.
> But here we only used for reset and disable the hardware.
>
> I think it is more straightforward and simple when we just check
> status register after write the reset or disable the ISI control
> register. Using interrupt here is over designed.
I still don't see how you're gaining from this. If you were writing new
code and you would say: "you know, this path will only be hit rarely, we
don't know whether interrupt would work, but we have here this simple
polling solution, that works" I would understand. But you already have a
technically-superior solution in place! And it works, right? Why change
it? On the one hand it's your hardware and it's up to you which direction
to develop it, and my task is just to help you integrate your work into
the mainline. On the other hand I really don't see any benefits, sorry.
I'd prefer not to do this unless you can explain the benefits.
> for the sake of performance I should use cpu_releax() instead of
> msleep() when waiting for the status bit.
Don't think that's a good idea. cpu_relax() allows the scheduler to switch
over to a different task, but if none is active, it'll come back instead
of going to sleep. Unless of course your event really kicks in very
quickly - much faster than 1ms. But depending on your architecture
implementation, msleep(1) might also be implemented as a busy look, then
cpu_relax() would be better.
Thanks
Guennadi
>
> Best Regards,
> Josh Wu
>
> >
> > Thanks
> > Guennadi
> >
> >>
> >> Signed-off-by: Josh Wu <rainyfeeling@gmail.com>
> >> ---
> >>
> >> drivers/media/platform/soc_camera/atmel-isi.c | 59 ++++++---------------------
> >> 1 file changed, 13 insertions(+), 46 deletions(-)
> >>
> >> diff --git a/drivers/media/platform/soc_camera/atmel-isi.c b/drivers/media/platform/soc_camera/atmel-isi.c
> >> index f0508ea..4ddc309 100644
> >> --- a/drivers/media/platform/soc_camera/atmel-isi.c
> >> +++ b/drivers/media/platform/soc_camera/atmel-isi.c
> >> @@ -12,7 +12,6 @@
> >> */
> >>
> >> #include <linux/clk.h>
> >> -#include <linux/completion.h>
> >> #include <linux/delay.h>
> >> #include <linux/fs.h>
> >> #include <linux/init.h>
> >> @@ -81,7 +80,6 @@ struct atmel_isi {
> >> struct isi_dma_desc dma_desc[MAX_BUFFER_NUM];
> >> bool enable_preview_path;
> >>
> >> - struct completion complete;
> >> /* ISI peripherial clock */
> >> struct clk *pclk;
> >> unsigned int irq;
> >> @@ -281,51 +279,14 @@ static irqreturn_t isi_interrupt(int irq, void *dev_id)
> >> mask = isi_readl(isi, ISI_INTMASK);
> >> pending = status & mask;
> >>
> >> - 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 (likely(pending & ISI_SR_CXFR_DONE) ||
> >> - likely(pending & ISI_SR_PXFR_DONE))
> >> - ret = atmel_isi_handle_streaming(isi);
> >> - }
> >> + if (likely(pending & ISI_SR_CXFR_DONE) ||
> >> + likely(pending & ISI_SR_PXFR_DONE))
> >> + ret = atmel_isi_handle_streaming(isi);
> >>
> >> spin_unlock(&isi->lock);
> >> return ret;
> >> }
> >>
> >> -#define WAIT_ISI_RESET 1
> >> -#define WAIT_ISI_DISABLE 0
> >> -static int atmel_isi_wait_status(struct atmel_isi *isi, int wait_reset)
> >> -{
> >> - unsigned long timeout;
> >> - /*
> >> - * The reset or disable will only succeed if we have a
> >> - * pixel clock from the camera.
> >> - */
> >> - init_completion(&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->complete,
> >> - msecs_to_jiffies(500));
> >> - if (timeout == 0)
> >> - return -ETIMEDOUT;
> >> -
> >> - return 0;
> >> -}
> >> -
> >> /* ------------------------------------------------------------------
> >> Videobuf operations
> >> ------------------------------------------------------------------*/
> >> @@ -493,8 +454,11 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
> >> pm_runtime_get_sync(ici->v4l2_dev.dev);
> >>
> >> /* Reset ISI */
> >> - ret = atmel_isi_wait_status(isi, WAIT_ISI_RESET);
> >> - if (ret < 0) {
> >> + isi_writel(isi, ISI_CTRL, ISI_CTRL_SRST);
> >> +
> >> + /* Check Reset status */
> >> + ret = isi_hw_wait_status(isi, ISI_CTRL_SRST, 500);
> >> + if (ret) {
> >> dev_err(icd->parent, "Reset ISI timed out\n");
> >> pm_runtime_put(ici->v4l2_dev.dev);
> >> return ret;
> >> @@ -549,8 +513,11 @@ static void stop_streaming(struct vb2_queue *vq)
> >> ISI_SR_CXFR_DONE | ISI_SR_PXFR_DONE);
> >>
> >> /* Disable ISI and wait for it is done */
> >> - ret = atmel_isi_wait_status(isi, WAIT_ISI_DISABLE);
> >> - if (ret < 0)
> >> + isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
> >> +
> >> + /* Check Reset status */
> >> + ret = isi_hw_wait_status(isi, ISI_CTRL_DIS, 500);
> >> + if (ret)
> >> dev_err(icd->parent, "Disable ISI timed out\n");
> >>
> >> pm_runtime_put(ici->v4l2_dev.dev);
> >> --
> >> 1.9.1
> >>
>
^ permalink raw reply [flat|nested] 29+ messages in thread