Re: [PATCH 3/3 v5] v4l: Add v4l2 subdev driver for S5P/EXYNOS4 MIPI-CSI receivers
Hi Laurent,
thanks again for your review.
On 05/14/2011 05:29 PM, Laurent Pinchart wrote:
On Wednesday 11 May 2011 17:17:10 Sylwester Nawrocki wrote:
Add the subdev driver for the MIPI CSIS units available in S5P and
Exynos4 SoC series. This driver supports both CSIS0 and CSIS1
MIPI-CSI2 receivers.
The driver requires Runtime PM to be enabled for proper operation.
Signed-off-by: Sylwester Nawrockis.nawro...@samsung.com
Signed-off-by: Kyungmin Parkkyungmin.p...@samsung.com
[snip]
diff --git a/drivers/media/video/s5p-fimc/mipi-csis.c
b/drivers/media/video/s5p-fimc/mipi-csis.c new file mode 100644
index 000..d50efcb
--- /dev/null
+++ b/drivers/media/video/s5p-fimc/mipi-csis.c
@@ -0,0 +1,722 @@
[snip]
+static void s5pcsis_enable_interrupts(struct csis_state *state, bool on)
+{
+u32 val = s5pcsis_read(state, S5PCSIS_INTMSK);
+
+val = on ? val | S5PCSIS_INTMSK_EN_ALL :
+ val ~S5PCSIS_INTMSK_EN_ALL;
+s5pcsis_write(state, S5PCSIS_INTMSK, val);
Shouldn't you clear all interrupt flags by writing to S5PCSIS_INTSRC before
enabling interrupts, just in case ?
In the start streaming sequence the device is first reset, then the receiver
and PHY is enabled and finally interrupts are enabled.
All interrupt sources are by default disabled after reset.
Enabling interrupts is the last thing done in the start streaming sequence.
By writing to S5PCSIS_INTSRC here any raised interrupt could be cleared
and possibly lost before being handled.
+}
[snip]
+static void s5pcsis_set_hsync_settle(struct csis_state *state, int settle)
+{
+u32 val = s5pcsis_read(state, S5PCSIS_DPHYCTRL);
+
+val= ~S5PCSIS_DPHYCTRL_HSS_MASK | (settle 27);
Do you mean
val = (val ~S5PCSIS_DPHYCTRL_HSS_MASK) | (settle 27);
?
Huh, naturally, yes, that was my intention.. Thank you for spotting this.
I used to have problems before when the settle time parameter wasn't
set properly, looks like the new boards are more tolerant..
+s5pcsis_write(state, S5PCSIS_DPHYCTRL, val);
+}
[snip]
+static int s5pcsis_set_fmt(struct v4l2_subdev *sd, struct v4l2_subdev_fh
*fh, + struct v4l2_subdev_format *fmt)
+{
+struct csis_state *state = sd_to_csis_state(sd);
+struct csis_pix_format const *csis_fmt;
+struct v4l2_mbus_framefmt *mf;
+
+mf = __s5pcsis_get_format(state, fh, fmt-pad, fmt-which);
+
+if (fmt-pad == CSIS_PAD_SOURCE) {
+if (mf) {
+mutex_lock(state-lock);
+fmt-format = *mf;
+mutex_unlock(state-lock);
+}
+return 0;
+}
+csis_fmt = s5pcsis_try_format(fmt-format);
+if (mf) {
+mutex_lock(state-lock);
+*mf = fmt-format;
+if (mf ==state-format) /* Store the active format */
I would replace the test by
if (fmt-which == V4L2_SUBDEV_FORMAT_TRY)
It's more explicit.
I agree, I'll change that.
+state-csis_fmt = csis_fmt;
+mutex_unlock(state-lock);
+}
+return 0;
+}
[snip]
+static int s5pcsis_suspend(struct device *dev)
+{
+struct s5p_platform_mipi_csis *pdata = dev-platform_data;
+struct platform_device *pdev = to_platform_device(dev);
+struct v4l2_subdev *sd = platform_get_drvdata(pdev);
+struct csis_state *state = sd_to_csis_state(sd);
+int ret;
+
+v4l2_dbg(1, debug, sd, %s: flags: 0x%x\n,
+ __func__, state-flags);
+
+mutex_lock(state-lock);
+if (state-flags ST_POWERED) {
+s5pcsis_stop_stream(state);
+ret = pdata-phy_enable(state-pdev, false);
+if (ret)
+goto unlock;
+
+if (state-supply regulator_disable(state-supply))
+goto unlock;
+
+clk_disable(state-clock[CSIS_CLK_GATE]);
+state-flags= ~ST_POWERED;
+}
+state-flags |= ST_SUSPENDED;
+ unlock:
+mutex_unlock(state-lock);
+return ret ? -EAGAIN : 0;
+}
+
+static int s5pcsis_resume(struct device *dev)
+{
+struct s5p_platform_mipi_csis *pdata = dev-platform_data;
+struct platform_device *pdev = to_platform_device(dev);
+struct v4l2_subdev *sd = platform_get_drvdata(pdev);
+struct csis_state *state = sd_to_csis_state(sd);
+int ret = 0;
+
+v4l2_dbg(1, debug, sd, %s: flags: 0x%x\n,
+ __func__, state-flags);
+
+mutex_lock(state-lock);
+if (!(state-flags ST_SUSPENDED))
+goto unlock;
+
+if (!(state-flags ST_POWERED)) {
If the device wasn't powered before being suspended, it should stay powered
off.
I'm not sure, shortly after system wide resume the device is powered off by
PM runtime core anyway.
There is no other means in this driver to enable power except using pm_runtime_*
calls. The device is being powered on or off only through these runtime PM
helpers, i.e.
I80 interface on S3C and manual command mode
Hi, I'm working on porting a kernel to a device which has an epaper-controller connected to the I80 interface of an S3C2416 and uses its manual command mode, via SIFCCON[0,1,2], as the datasheet calls it. While digging around the mailinglists I found patches by Inki Dae that implement the I80 basics. (cpu_mode struct and probe_win). As there are multiple versions available in the fbdev patchwork and the i80 interface seems to have been dropped in the latest version from 2011-04 I'd like ask if there are final versions laying around somewhere. Also, to not reinvent the wheel, does an implementation of the i80 interface [via SIFCCONx] for s3c-fb exist somewhere per chance? Thanks Heiko -- To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3 v5] v4l: Add v4l2 subdev driver for S5P/EXYNOS4 MIPI-CSI receivers
Hi Sylwester,
On Sunday 15 May 2011 11:33:14 Sylwester Nawrocki wrote:
On 05/14/2011 05:29 PM, Laurent Pinchart wrote:
On Wednesday 11 May 2011 17:17:10 Sylwester Nawrocki wrote:
[snip]
+static int s5pcsis_suspend(struct device *dev)
+{
+ struct s5p_platform_mipi_csis *pdata = dev-platform_data;
+ struct platform_device *pdev = to_platform_device(dev);
+ struct v4l2_subdev *sd = platform_get_drvdata(pdev);
+ struct csis_state *state = sd_to_csis_state(sd);
+ int ret;
+
+ v4l2_dbg(1, debug, sd, %s: flags: 0x%x\n,
+ __func__, state-flags);
+
+ mutex_lock(state-lock);
+ if (state-flags ST_POWERED) {
+ s5pcsis_stop_stream(state);
+ ret = pdata-phy_enable(state-pdev, false);
+ if (ret)
+ goto unlock;
+
+ if (state-supply regulator_disable(state-supply))
+ goto unlock;
+
+ clk_disable(state-clock[CSIS_CLK_GATE]);
+ state-flags= ~ST_POWERED;
+ }
+ state-flags |= ST_SUSPENDED;
+ unlock:
+ mutex_unlock(state-lock);
+ return ret ? -EAGAIN : 0;
+}
+
+static int s5pcsis_resume(struct device *dev)
+{
+ struct s5p_platform_mipi_csis *pdata = dev-platform_data;
+ struct platform_device *pdev = to_platform_device(dev);
+ struct v4l2_subdev *sd = platform_get_drvdata(pdev);
+ struct csis_state *state = sd_to_csis_state(sd);
+ int ret = 0;
+
+ v4l2_dbg(1, debug, sd, %s: flags: 0x%x\n,
+ __func__, state-flags);
+
+ mutex_lock(state-lock);
+ if (!(state-flags ST_SUSPENDED))
+ goto unlock;
+
+ if (!(state-flags ST_POWERED)) {
If the device wasn't powered before being suspended, it should stay
powered off.
I'm not sure, shortly after system wide resume the device is powered off by
PM runtime core anyway.
There is no other means in this driver to enable power except using
pm_runtime_* calls. The device is being powered on or off only through
these runtime PM helpers, i.e. s5pcsis_resume/s5pcsis_suspend.
(full source can be found here: http://tinyurl.com/6fozx34)
OK, it should be fine then.
The pm_runtime_resume helper is guaranteed by the PM core to be called only
on device in 'suspended' state.
From Documentation/power/runtime_pm.txt:
...
* Once the subsystem-level resume callback has completed successfully, the
PM core regards the device as fully operational, which means that the
device _must_ be able to complete I/O operations as needed. The run-time
PM status of the device is then 'active'.
...
If s5pcsis_resume would return 0 without really enabling device clocks and
the external voltage regulator then the runtime PM core idea about the
device's state would be wrong.
I'm not a PM expert but documentation says that it's better to leave
device fully functional after system wide driver's resume() helper returns.
From Documentation/power/devices.txt:
...
When resuming from standby or memory sleep, the phases are:
resume_noirq, resume, complete.
(...)
At the end of these phases, drivers should be as functional as they were
before suspending: I/O can be performed using DMA and IRQs, and the
relevant clocks are gated on. Even if the device was in a low-power state
before the system sleep because of runtime power management, afterwards it
should be back in its full-power state. There are multiple reasons why
it's best to do this; they are discussed in more detail in
Documentation/power/runtime_pm.txt.
...
Unfortunately there seem to be no standard way to instruct the PM core to
enable power of a few (I2C/client platform) devices forming the video
pipeline in an arbitrary order. The parent devices of my platforms devices
are already the power domain devices.
So it might be a good idea to forbid enabling sub-device's power when
the host driver is not using it, to have full control of the pipeline
devices power enable sequence at any time.
I perhaps need to isolate functions out from of s5pcsis_resume/suspend and
then call that in s_power op and s5pcsis_resume/suspend. Don't really like
this idea though... It would virtually render pm_runtime_* calls unusable
in this sub-device driver, those would make sense only in the host driver..
I think using the pm_runtime_* calls make sense, they could replace the subdev
s_power operation in the long term. We'll need to evaluate that (I'm not sure
if runtime PM is available on all platforms targetted by V4L2 for instance).
I just wanted to put all what is needed to control device's power in the PM
helpers and then use pm_runtime_* calls where required.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] ARM: S5PV210: Fix possible null pointer dereference.
Sergei Shtylyov wrote:
Hello.
Hi ;)
On 13-05-2011 11:03, Kukjin Kim wrote:
From: Jonghwan Choijhbird.c...@samsung.com
Signed-off-by: Jonghwan Choijhbird.c...@samsung.com
Signed-off-by: Kukjin Kimkgene@samsung.com
---
arch/arm/mach-s5pv210/cpufreq.c |4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/arch/arm/mach-s5pv210/cpufreq.c b/arch/arm/mach-
s5pv210/cpufreq.c
index 22046e2..58340ee 100644
--- a/arch/arm/mach-s5pv210/cpufreq.c
+++ b/arch/arm/mach-s5pv210/cpufreq.c
@@ -105,8 +105,10 @@ static void s5pv210_set_refresh(enum
s5pv210_dmc_port ch, unsigned long freq)
reg = (S5P_VA_DMC0 + 0x30);
else if (ch == DMC1)
reg = (S5P_VA_DMC1 + 0x30);
- else
+ else {
printk(KERN_ERR Cannot find DMC port\n);
+ return;
+ }
According to CodingStyle, other branches should now have {} too.
Yes, you're right.
I updated its commit in my tree like following.
---
diff --git a/arch/arm/mach-s5pv210/cpufreq.c
b/arch/arm/mach-s5pv210/cpufreq.c
index 22046e2..153af8b 100644
--- a/arch/arm/mach-s5pv210/cpufreq.c
+++ b/arch/arm/mach-s5pv210/cpufreq.c
@@ -101,12 +101,14 @@ static void s5pv210_set_refresh(enum s5pv210_dmc_port
ch, unsigned long freq)
unsigned long tmp, tmp1;
void __iomem *reg = NULL;
- if (ch == DMC0)
+ if (ch == DMC0) {
reg = (S5P_VA_DMC0 + 0x30);
- else if (ch == DMC1)
+ } else if (ch == DMC1) {
reg = (S5P_VA_DMC1 + 0x30);
- else
+ } else {
printk(KERN_ERR Cannot find DMC port\n);
+ return;
+ }
/* Find current DRAM frequency */
tmp = s5pv210_dram_conf[ch].freq;
---
Thanks.
Best regards,
Kgene.
--
Kukjin Kim kgene@samsung.com, Senior Engineer,
SW Solution Development Team, Samsung Electronics Co., Ltd.
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
