Re: [PATCH v2 1/3] media: atmel-isi: remove the useless code which disable isi
Hi Josh,
On Tue, 12 May 2015, Josh Wu wrote:
Hi, Guennadi
On 5/12/2015 4:16 AM, Guennadi Liakhovetski wrote:
Hi Josh,
Thanks for the patch. I'm afraid I don't quite understand why it is
needed, could you maybe explain a bit more? Is it because
(1) during ISI configuration the pixel clock from the sensor is (usually)
anyway disabled, so, we don't have to additionally disable the ISI,
without the pixel clock the ISI is anyway already disabled.
or
(2) disabling the ISI at those locations below breaks something, because
when the ISI is disabled, the functionality, that's later used isn't
available.
I assume it's (1), but if that's the case, then this patch is just
cosmetic, right?
Right, it is (1). This patch doesn't impact the function of isi.
The point is when we disable the ISI, we need make sure ISI peripheral clock
and pixel clock is enabled. And also need to check the status register or irq
to know whether the disable operation is successful. So best way to disable
ISI is to call atmel_isi_wait_status(isi, WAIT_ISI_DISABLE).
From this point, the isi_write(isi, ISI_CTRL, ISI_CTRL_DIS) in
atmel_isi_probe() is not useful as ISI peripheral clock is not enable yet.
but the other two in configure_geometry(), isi_camera_set_bus_param() can work
and may protect the case the ISI is not in off state.
Ok, thanks for the explanation.
The ISI is anyway disabled, so, that operation simply has
no effect, but also doesn't hurt. OTOH, if some sensor keeps its master
clock running all the time, then switching the ISI off as in the current
version helps save some power, unless it breaks anything?
So what about to keep the isi_write(isi, ISI_CTRL, ISI_CTRL_DIS) in
configure_geometry() and isi_camera_set_bus_param().
Only remove it from atmel_isi_probe()?
Good, let's do that.
Thanks
Guennadi
Best Regards,
Josh Wu
Thanks
Guennadi
On Thu, 9 Apr 2015, Josh Wu wrote:
To program ISI control register, the pixel clock should be enabled.
So without pixel clock (from sensor) enabled, disable ISI controller is
not make sense. So this patch remove those code.
Signed-off-by: Josh Wu josh...@atmel.com
---
Changes in v2:
- this file is new added.
drivers/media/platform/soc_camera/atmel-isi.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
b/drivers/media/platform/soc_camera/atmel-isi.c
index c125b1d..31254b4 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -131,8 +131,6 @@ static int configure_geometry(struct atmel_isi *isi,
u32 width,
return -EINVAL;
}
- isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
-
cfg2 = isi_readl(isi, ISI_CFG2);
/* Set YCC swap mode */
cfg2 = ~ISI_CFG2_YCC_SWAP_MODE_MASK;
@@ -843,7 +841,6 @@ static int isi_camera_set_bus_param(struct
soc_camera_device *icd)
cfg1 |= ISI_CFG1_THMASK_BEATS_16;
- isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
isi_writel(isi, ISI_CFG1, cfg1);
return 0;
@@ -1022,8 +1019,6 @@ static int atmel_isi_probe(struct platform_device
*pdev)
if (isi-pdata.data_width_flags ISI_DATAWIDTH_10)
isi-width_flags |= 1 9;
- isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
-
irq = platform_get_irq(pdev, 0);
if (IS_ERR_VALUE(irq)) {
ret = irq;
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] media: atmel-isi: remove the useless code which disable isi
Hi Josh,
Thanks for the patch. I'm afraid I don't quite understand why it is
needed, could you maybe explain a bit more? Is it because
(1) during ISI configuration the pixel clock from the sensor is (usually)
anyway disabled, so, we don't have to additionally disable the ISI,
without the pixel clock the ISI is anyway already disabled.
or
(2) disabling the ISI at those locations below breaks something, because
when the ISI is disabled, the functionality, that's later used isn't
available.
I assume it's (1), but if that's the case, then this patch is just
cosmetic, right? The ISI is anyway disabled, so, that operation simply has
no effect, but also doesn't hurt. OTOH, if some sensor keeps its master
clock running all the time, then switching the ISI off as in the current
version helps save some power, unless it breaks anything?
Thanks
Guennadi
On Thu, 9 Apr 2015, Josh Wu wrote:
To program ISI control register, the pixel clock should be enabled.
So without pixel clock (from sensor) enabled, disable ISI controller is
not make sense. So this patch remove those code.
Signed-off-by: Josh Wu josh...@atmel.com
---
Changes in v2:
- this file is new added.
drivers/media/platform/soc_camera/atmel-isi.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
b/drivers/media/platform/soc_camera/atmel-isi.c
index c125b1d..31254b4 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -131,8 +131,6 @@ static int configure_geometry(struct atmel_isi *isi, u32
width,
return -EINVAL;
}
- isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
-
cfg2 = isi_readl(isi, ISI_CFG2);
/* Set YCC swap mode */
cfg2 = ~ISI_CFG2_YCC_SWAP_MODE_MASK;
@@ -843,7 +841,6 @@ static int isi_camera_set_bus_param(struct
soc_camera_device *icd)
cfg1 |= ISI_CFG1_THMASK_BEATS_16;
- isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
isi_writel(isi, ISI_CFG1, cfg1);
return 0;
@@ -1022,8 +1019,6 @@ static int atmel_isi_probe(struct platform_device *pdev)
if (isi-pdata.data_width_flags ISI_DATAWIDTH_10)
isi-width_flags |= 1 9;
- isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
-
irq = platform_get_irq(pdev, 0);
if (IS_ERR_VALUE(irq)) {
ret = irq;
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] media: atmel-isi: remove the useless code which disable isi
Hi, Guennadi
On 5/12/2015 4:16 AM, Guennadi Liakhovetski wrote:
Hi Josh,
Thanks for the patch. I'm afraid I don't quite understand why it is
needed, could you maybe explain a bit more? Is it because
(1) during ISI configuration the pixel clock from the sensor is (usually)
anyway disabled, so, we don't have to additionally disable the ISI,
without the pixel clock the ISI is anyway already disabled.
or
(2) disabling the ISI at those locations below breaks something, because
when the ISI is disabled, the functionality, that's later used isn't
available.
I assume it's (1), but if that's the case, then this patch is just
cosmetic, right?
Right, it is (1). This patch doesn't impact the function of isi.
The point is when we disable the ISI, we need make sure ISI peripheral
clock and pixel clock is enabled. And also need to check the status
register or irq to know whether the disable operation is successful. So
best way to disable ISI is to call atmel_isi_wait_status(isi,
WAIT_ISI_DISABLE).
From this point, the isi_write(isi, ISI_CTRL, ISI_CTRL_DIS) in
atmel_isi_probe() is not useful as ISI peripheral clock is not enable yet.
but the other two in configure_geometry(), isi_camera_set_bus_param()
can work and may protect the case the ISI is not in off state.
The ISI is anyway disabled, so, that operation simply has
no effect, but also doesn't hurt. OTOH, if some sensor keeps its master
clock running all the time, then switching the ISI off as in the current
version helps save some power, unless it breaks anything?
So what about to keep the isi_write(isi, ISI_CTRL, ISI_CTRL_DIS) in
configure_geometry() and isi_camera_set_bus_param().
Only remove it from atmel_isi_probe()?
Best Regards,
Josh Wu
Thanks
Guennadi
On Thu, 9 Apr 2015, Josh Wu wrote:
To program ISI control register, the pixel clock should be enabled.
So without pixel clock (from sensor) enabled, disable ISI controller is
not make sense. So this patch remove those code.
Signed-off-by: Josh Wu josh...@atmel.com
---
Changes in v2:
- this file is new added.
drivers/media/platform/soc_camera/atmel-isi.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
b/drivers/media/platform/soc_camera/atmel-isi.c
index c125b1d..31254b4 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -131,8 +131,6 @@ static int configure_geometry(struct atmel_isi *isi, u32
width,
return -EINVAL;
}
- isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
-
cfg2 = isi_readl(isi, ISI_CFG2);
/* Set YCC swap mode */
cfg2 = ~ISI_CFG2_YCC_SWAP_MODE_MASK;
@@ -843,7 +841,6 @@ static int isi_camera_set_bus_param(struct
soc_camera_device *icd)
cfg1 |= ISI_CFG1_THMASK_BEATS_16;
- isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
isi_writel(isi, ISI_CFG1, cfg1);
return 0;
@@ -1022,8 +1019,6 @@ static int atmel_isi_probe(struct platform_device *pdev)
if (isi-pdata.data_width_flags ISI_DATAWIDTH_10)
isi-width_flags |= 1 9;
- isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
-
irq = platform_get_irq(pdev, 0);
if (IS_ERR_VALUE(irq)) {
ret = irq;
--
1.9.1
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] media: atmel-isi: remove the useless code which disable isi
Hi, Laurent
On 4/12/2015 9:10 PM, Laurent Pinchart wrote:
Hi Josh,
Thank you for the patch.
On Thursday 09 April 2015 17:01:46 Josh Wu wrote:
To program ISI control register, the pixel clock should be enabled.
That's an awful hardware design :-(
yes, But I need to live with this.
So without pixel clock (from sensor) enabled, disable ISI controller is
not make sense. So this patch remove those code.
Signed-off-by: Josh Wu josh...@atmel.com
---
Changes in v2:
- this file is new added.
drivers/media/platform/soc_camera/atmel-isi.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
b/drivers/media/platform/soc_camera/atmel-isi.c index c125b1d..31254b4
100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -131,8 +131,6 @@ static int configure_geometry(struct atmel_isi *isi, u32
width, return -EINVAL;
}
- isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
-
cfg2 = isi_readl(isi, ISI_CFG2);
Can the configuration registers be accessed when the pixel clock is disabled ?
yes, it can be accessed. The CFG1, CFG2 are not impacted.
So far as I know only the ISI_CR is impacted, we can
disable/enable/reset ISI by set ISI_CR.
Since ISI_CR register will work with pixel clock, so you need to poll
the ISI_SR to verify your control of ISI is effective.
Best Regards,
Josh Wu
/* Set YCC swap mode */
cfg2 = ~ISI_CFG2_YCC_SWAP_MODE_MASK;
@@ -843,7 +841,6 @@ static int isi_camera_set_bus_param(struct
soc_camera_device *icd)
cfg1 |= ISI_CFG1_THMASK_BEATS_16;
- isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
isi_writel(isi, ISI_CFG1, cfg1);
return 0;
@@ -1022,8 +1019,6 @@ static int atmel_isi_probe(struct platform_device
*pdev) if (isi-pdata.data_width_flags ISI_DATAWIDTH_10)
isi-width_flags |= 1 9;
- isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
-
irq = platform_get_irq(pdev, 0);
if (IS_ERR_VALUE(irq)) {
ret = irq;
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/3] media: atmel-isi: remove the useless code which disable isi
Hi Josh,
Thank you for the patch.
On Thursday 09 April 2015 17:01:46 Josh Wu wrote:
To program ISI control register, the pixel clock should be enabled.
That's an awful hardware design :-(
So without pixel clock (from sensor) enabled, disable ISI controller is
not make sense. So this patch remove those code.
Signed-off-by: Josh Wu josh...@atmel.com
---
Changes in v2:
- this file is new added.
drivers/media/platform/soc_camera/atmel-isi.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
b/drivers/media/platform/soc_camera/atmel-isi.c index c125b1d..31254b4
100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -131,8 +131,6 @@ static int configure_geometry(struct atmel_isi *isi, u32
width, return -EINVAL;
}
- isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
-
cfg2 = isi_readl(isi, ISI_CFG2);
Can the configuration registers be accessed when the pixel clock is disabled ?
/* Set YCC swap mode */
cfg2 = ~ISI_CFG2_YCC_SWAP_MODE_MASK;
@@ -843,7 +841,6 @@ static int isi_camera_set_bus_param(struct
soc_camera_device *icd)
cfg1 |= ISI_CFG1_THMASK_BEATS_16;
- isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
isi_writel(isi, ISI_CFG1, cfg1);
return 0;
@@ -1022,8 +1019,6 @@ static int atmel_isi_probe(struct platform_device
*pdev) if (isi-pdata.data_width_flags ISI_DATAWIDTH_10)
isi-width_flags |= 1 9;
- isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
-
irq = platform_get_irq(pdev, 0);
if (IS_ERR_VALUE(irq)) {
ret = irq;
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/3] media: atmel-isi: remove the useless code which disable isi
To program ISI control register, the pixel clock should be enabled.
So without pixel clock (from sensor) enabled, disable ISI controller is
not make sense. So this patch remove those code.
Signed-off-by: Josh Wu josh...@atmel.com
---
Changes in v2:
- this file is new added.
drivers/media/platform/soc_camera/atmel-isi.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/media/platform/soc_camera/atmel-isi.c
b/drivers/media/platform/soc_camera/atmel-isi.c
index c125b1d..31254b4 100644
--- a/drivers/media/platform/soc_camera/atmel-isi.c
+++ b/drivers/media/platform/soc_camera/atmel-isi.c
@@ -131,8 +131,6 @@ static int configure_geometry(struct atmel_isi *isi, u32
width,
return -EINVAL;
}
- isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
-
cfg2 = isi_readl(isi, ISI_CFG2);
/* Set YCC swap mode */
cfg2 = ~ISI_CFG2_YCC_SWAP_MODE_MASK;
@@ -843,7 +841,6 @@ static int isi_camera_set_bus_param(struct
soc_camera_device *icd)
cfg1 |= ISI_CFG1_THMASK_BEATS_16;
- isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
isi_writel(isi, ISI_CFG1, cfg1);
return 0;
@@ -1022,8 +1019,6 @@ static int atmel_isi_probe(struct platform_device *pdev)
if (isi-pdata.data_width_flags ISI_DATAWIDTH_10)
isi-width_flags |= 1 9;
- isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
-
irq = platform_get_irq(pdev, 0);
if (IS_ERR_VALUE(irq)) {
ret = irq;
--
1.9.1
--
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
