Re: [PATCH v4 02/10] ARM: dts: list the CPU nodes for Exynos5250
On 2 September 2013 12:07, Chander Kashyap chander.kash...@linaro.org wrote: I tested on 3.11-rc7. Both the cpu's are booting. I didnet see any issue. Oops!! I copied dtb to wrong location.. stupid mistake.. sorry. -- 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: [RFC PATCH 6/6] spi: spi-s3c64xx: Move DMA initialization
On 09/09/2013 04:53 PM, Mark Brown wrote: On Mon, Sep 09, 2013 at 04:09:26PM +0200, Lukasz Czerwinski wrote: This patch removes DMA channel initialization and deinitialization for each transfer. Now DMA channel is requested only at the driver initialization and stored in driver data. So, this is fine but I had been under the impression that one of the reasons that the channels were only being requested at transfer time was that they could be used for other purposes too. If that is not the case then fine, if that is the case then probably moving to prepare/unprepare transfer hardware (when the clocks are enabled) would make more sense - that'd avoid repeated allocations and frees while the controller is busy which should be the main issue performance wise. For 16kB buffer upload time is reduced from ~90ms to ~80ms (I tested with S5C73M3 355560B transfer). If you think that performance increase isn't valuable we should skip that patch. amba-pl80 uses virtual DMA channels support so driver won't occupy all the time physical channel. pl330 driver don't use such framework but It will be very hard to occupy all channels at the same time. If I'm wrong please correct me. Thanks Lukasz -- 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: [RFC PATCH 3/6] spi: spi-s3c64xx: Add coherent buffers for DMA transfers
On 09/09/2013 04:45 PM, Mark Brown wrote: On Mon, Sep 09, 2013 at 04:09:23PM +0200, Lukasz Czerwinski wrote: The spi-s3c64xx currently doesn't support transfers from non-contiguous client buffers. This patch adds two coherent buffers which allow transfers from non-contiguous client buffers without extra coherent memory allocation in the client driver. Buffer size is hardcoded to 16kB for Tx/Rx. Client drivers shouldn't exceed that value. This seems like a very low limit to have, consider things like firmware downloads for example. It seems reasonable to have a preallocated small buffer but there should be some fallback for larger transfer sizes. I have tested my modification with different buffer sizes with S5C73M3 driver (355560 B upload). I obtained following upload times: 16kB buffer: - 83.972 ms - 79.196 ms - 79.432 ms 128kB buffer: - 74.449 ms - 80.719 ms - 75.599 ms For 256kB I obtained similar results as in 128kB case. Normally non-interrupted SPI transfer should take 56ms (50MHz). Performance loss is approximately about 6% between 16kB and 128KB buffer. I my opinion there are no need to use bigger buffers. I propose add extra module parameter which allows to change buffer size. I also didn't notice any checks in the code for the length of transfers so this will corrupt memory if a client driver tries to transfer more than the preallocated buffer as things stand. Right, I will add that. Thanks Lukasz -- 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: [RFC PATCH 3/6] spi: spi-s3c64xx: Add coherent buffers for DMA transfers
On Tue, Sep 10, 2013 at 01:23:45PM +0200, Łukasz Czerwiński wrote: On 09/09/2013 04:45 PM, Mark Brown wrote: This seems like a very low limit to have, consider things like firmware downloads for example. It seems reasonable to have a preallocated small buffer but there should be some fallback for larger transfer sizes. I have tested my modification with different buffer sizes with S5C73M3 driver (355560 B upload). I obtained following upload times: 16kB buffer: - 83.972 ms - 79.196 ms - 79.432 ms 128kB buffer: - 74.449 ms - 80.719 ms - 75.599 ms For 256kB I obtained similar results as in 128kB case. Normally non-interrupted SPI transfer should take 56ms (50MHz). Performance loss is approximately about 6% between 16kB and 128KB buffer. I my opinion there are no need to use bigger buffers. You're missing the point here. This requires the client driver to know the maximum transfer size that the controller supports and split the transfer up for it. This isn't something that the SPI API supports now and while it would be useful to add for controllers that are genuinely limited in transfer size it doesn't seem sane to just randomly impose a limit to make life easier for software. Not all applications will be able to split their transfers up in the first place and it seems like the wrong place to put that knowledge. What I would expect to see the driver doing here is either allocating a larger buffer on demand or (probably more reliably given the need for contiguous physical memory) transparently splitting larger transfers. A super smart implementation would overlap the copy of blocks with the transfers of preceeding blocks, and there should be something we can do to avoid having to copy data that's already in suitable memory. Ideally this would all be factored out into the core based on limit information but that's probably not required immediately. Though now I write that I'm wondering how many other devices should be doing this but aren't. The DMA mapping of transfer buffers doesn't seem terribly device specific... I propose add extra module parameter which allows to change buffer size. No, module parameters are not sensible for modern code. If you wanted runtime configuration a sysfs tunable would be better though I do think that should be tuning optimisation, not a hard limit on the transfer size. signature.asc Description: Digital signature
Re: [RFC PATCH 6/6] spi: spi-s3c64xx: Move DMA initialization
On Tue, Sep 10, 2013 at 01:24:01PM +0200, Łukasz Czerwiński wrote: For 16kB buffer upload time is reduced from ~90ms to ~80ms (I tested with S5C73M3 355560B transfer). If you think that performance increase isn't valuable we should skip that patch. That does seem worthwhile. Is that just a single transfer in isolation? amba-pl80 uses virtual DMA channels support so driver won't occupy all the time physical channel. pl330 driver don't use such framework but It will be very hard to occupy all channels at the same time. If I'm wrong please correct me. OK. signature.asc Description: Digital signature
Re: [PATCH v2 3/5] [media] exynos-mscl: Add m2m functionality for the M-Scaler driver
Hi Sylwester,
Almost all of the comments are already addressed.
Will try to post the v3 by tomorrow.
I have one doubt?
Do I need to rebase this driver on m2m-helpers-v2 or once the driver
is merged we can take this up?
Regards,
Shaik Ameer Basha
On Thu, Aug 29, 2013 at 6:51 PM, Sylwester Nawrocki
s.nawro...@samsung.com wrote:
On 08/19/2013 12:58 PM, Shaik Ameer Basha wrote:
This patch adds the memory to memory (m2m) interface functionality
for the M-Scaler driver.
Signed-off-by: Shaik Ameer Basha shaik.am...@samsung.com
---
drivers/media/platform/exynos-mscl/mscl-m2m.c | 763
+
1 file changed, 763 insertions(+)
create mode 100644 drivers/media/platform/exynos-mscl/mscl-m2m.c
diff --git a/drivers/media/platform/exynos-mscl/mscl-m2m.c
b/drivers/media/platform/exynos-mscl/mscl-m2m.c
new file mode 100644
index 000..fecbb57
--- /dev/null
+++ b/drivers/media/platform/exynos-mscl/mscl-m2m.c
@@ -0,0 +1,763 @@
+/*
+ * Copyright (c) 2013 - 2014 Samsung Electronics Co., Ltd.
2013 - 2014 ??
+ * http://www.samsung.com
+ *
+ * Samsung EXYNOS5 SoC series M-Scaler driver
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published
+ * by the Free Software Foundation, either version 2 of the License,
+ * or (at your option) any later version.
+ */
+
+#include linux/module.h
+#include linux/pm_runtime.h
+#include linux/slab.h
+
+#include media/v4l2-ioctl.h
+
+#include mscl-core.h
+
+static int mscl_m2m_ctx_stop_req(struct mscl_ctx *ctx)
+{
+ struct mscl_ctx *curr_ctx;
+ struct mscl_dev *mscl = ctx-mscl_dev;
+ int ret;
+
+ curr_ctx = v4l2_m2m_get_curr_priv(mscl-m2m.m2m_dev);
+ if (!mscl_m2m_pending(mscl) || (curr_ctx != ctx))
+ return 0;
+
+ mscl_ctx_state_lock_set(MSCL_CTX_STOP_REQ, ctx);
+ ret = wait_event_timeout(mscl-irq_queue,
+ !mscl_ctx_state_is_set(MSCL_CTX_STOP_REQ, ctx),
+ MSCL_SHUTDOWN_TIMEOUT);
+
+ return ret == 0 ? -ETIMEDOUT : ret;
+}
+
+static int mscl_m2m_start_streaming(struct vb2_queue *q, unsigned int count)
+{
+ struct mscl_ctx *ctx = q-drv_priv;
+ int ret;
+
+ ret = pm_runtime_get_sync(ctx-mscl_dev-pdev-dev);
+
+ return ret 0 ? 0 : ret;
+}
+
+static int mscl_m2m_stop_streaming(struct vb2_queue *q)
+{
+ struct mscl_ctx *ctx = q-drv_priv;
+ int ret;
+
+ ret = mscl_m2m_ctx_stop_req(ctx);
+ if (ret == -ETIMEDOUT)
+ mscl_m2m_job_finish(ctx, VB2_BUF_STATE_ERROR);
+
+ pm_runtime_put(ctx-mscl_dev-pdev-dev);
+
+ return 0;
+}
+
+void mscl_m2m_job_finish(struct mscl_ctx *ctx, int vb_state)
+{
+ struct vb2_buffer *src_vb, *dst_vb;
+
+ if (!ctx || !ctx-m2m_ctx)
+ return;
+
+ src_vb = v4l2_m2m_src_buf_remove(ctx-m2m_ctx);
+ dst_vb = v4l2_m2m_dst_buf_remove(ctx-m2m_ctx);
+
+ if (src_vb dst_vb) {
+ v4l2_m2m_buf_done(src_vb, vb_state);
+ v4l2_m2m_buf_done(dst_vb, vb_state);
+
+ v4l2_m2m_job_finish(ctx-mscl_dev-m2m.m2m_dev,
+ ctx-m2m_ctx);
+ }
+}
+
+
Stray empty line.
+static void mscl_m2m_job_abort(void *priv)
+{
+ struct mscl_ctx *ctx = priv;
+ int ret;
+
+ ret = mscl_m2m_ctx_stop_req(ctx);
+ if (ret == -ETIMEDOUT)
+ mscl_m2m_job_finish(ctx, VB2_BUF_STATE_ERROR);
+}
+
+static int mscl_get_bufs(struct mscl_ctx *ctx)
+{
+ struct mscl_frame *s_frame, *d_frame;
+ struct vb2_buffer *src_vb, *dst_vb;
+ int ret;
+
+ s_frame = ctx-s_frame;
+ d_frame = ctx-d_frame;
+
+ src_vb = v4l2_m2m_next_src_buf(ctx-m2m_ctx);
+ ret = mscl_prepare_addr(ctx, src_vb, s_frame, s_frame-addr);
+ if (ret)
How about using if (ret 0) pattern consistently ?
+ return ret;
+
+ dst_vb = v4l2_m2m_next_dst_buf(ctx-m2m_ctx);
+ ret = mscl_prepare_addr(ctx, dst_vb, d_frame, d_frame-addr);
+ if (ret)
+ return ret;
+
+ dst_vb-v4l2_buf.timestamp = src_vb-v4l2_buf.timestamp;
+
+ return 0;
+}
+
+static void mscl_m2m_device_run(void *priv)
+{
+ struct mscl_ctx *ctx = priv;
+ struct mscl_dev *mscl;
+ unsigned long flags;
+ int ret;
+ bool is_set = false;
Unneeded initialization. And I can see a room for improvement WRT
the variable's name.
+
+ if (WARN(!ctx, null hardware context\n))
+ return;
+
+ mscl = ctx-mscl_dev;
+ spin_lock_irqsave(mscl-slock, flags);
+
+ set_bit(ST_M2M_PEND, mscl-state);
+
+ /* Reconfigure hardware if the context has changed. */
+ if (mscl-m2m.ctx != ctx) {
+ dev_dbg(mscl-pdev-dev,
+ mscl-m2m.ctx = 0x%p, current_ctx = 0x%p,
+ mscl-m2m.ctx, ctx);
+ ctx-state |=
Re: [RFC PATCH 6/6] spi: spi-s3c64xx: Move DMA initialization
On 09/10/2013 02:03 PM, Mark Brown wrote: On Tue, Sep 10, 2013 at 01:24:01PM +0200, Łukasz Czerwiński wrote: For 16kB buffer upload time is reduced from ~90ms to ~80ms (I tested with S5C73M3 355560B transfer). If you think that performance increase isn't valuable we should skip that patch. That does seem worthwhile. Is that just a single transfer in isolation? I gave you summary time when firmware is transferred via 22x16kB blocks. Each transfer was isolated. amba-pl80 uses virtual DMA channels support so driver won't occupy all the time physical channel. pl330 driver don't use such framework but It will be very hard to occupy all channels at the same time. If I'm wrong please correct me. OK. Thanks Lukasz -- 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: [RFC PATCH 6/6] spi: spi-s3c64xx: Move DMA initialization
On Tue, Sep 10, 2013 at 03:16:59PM +0200, Łukasz Czerwiński wrote: On 09/10/2013 02:03 PM, Mark Brown wrote: On Tue, Sep 10, 2013 at 01:24:01PM +0200, Łukasz Czerwiński wrote: For 16kB buffer upload time is reduced from ~90ms to ~80ms (I tested with S5C73M3 355560B transfer). If you think that performance increase isn't valuable we should skip that patch. That does seem worthwhile. Is that just a single transfer in isolation? I gave you summary time when firmware is transferred via 22x16kB blocks. Each transfer was isolated. OK, so that's the per-transfer number? I'm just wondering if moving it to prepare/unprepare would achieve the same effect here. signature.asc Description: Digital signature
Re: [PATCH] ARM: dts: Disable Exynos5250 I2S controllers by default
On Tue, Sep 10, 2013 at 06:35:48PM +0100, Mark Rutland wrote: It seems far more sensible to me to mark devices disabled by default in shared dtsi files and then okay them as needed in particular dts files. I'd be happy with more of this. Yeah, me too - though only for devices that have an external impact, for things that are internal only (eg, a crypto engine) it makes sense to enable them by default since they should normally be usable regardless of the system configuration. signature.asc Description: Digital signature
Re: [PATCH] ARM: dts: Disable Exynos5250 I2S controllers by default
On Fri, Sep 06, 2013 at 07:17:15PM +0100, Mark Brown wrote:
From: Mark Brown broo...@linaro.org
Rather than requiring each board to explicitly disable the I2S controllers
it is not using instead require boards to enable those that they are using.
This is required for audio operation on Arndale, one of the unused I2S
controllers is pinmuxed with the LDO enable GPIOs for the WM1811A.
Signed-off-by: Mark Brown broo...@linaro.org
---
This seems like a more robust approach to handling the externally
visible IPs - if this is OK I can go through and do further updates for
other devices.
Acked-by: Mark Rutland mark.rutl...@arm.com
It seems far more sensible to me to mark devices disabled by default in
shared dtsi files and then okay them as needed in particular dts files.
I'd be happy with more of this.
arch/arm/boot/dts/exynos5250-arndale.dts | 4
arch/arm/boot/dts/exynos5250-smdk5250.dts | 8
arch/arm/boot/dts/exynos5250.dtsi | 3 +++
3 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/arch/arm/boot/dts/exynos5250-arndale.dts
b/arch/arm/boot/dts/exynos5250-arndale.dts
index cee55fa..4687fa0 100644
--- a/arch/arm/boot/dts/exynos5250-arndale.dts
+++ b/arch/arm/boot/dts/exynos5250-arndale.dts
@@ -412,6 +412,10 @@
status = disabled;
};
+ i2s0: i2s@0383 {
+ status = okay;
+ };
+
spi_0: spi@12d2 {
status = disabled;
};
diff --git a/arch/arm/boot/dts/exynos5250-smdk5250.dts
b/arch/arm/boot/dts/exynos5250-smdk5250.dts
index 2538b32..f86d567 100644
--- a/arch/arm/boot/dts/exynos5250-smdk5250.dts
+++ b/arch/arm/boot/dts/exynos5250-smdk5250.dts
@@ -231,14 +231,6 @@
status = okay;
};
- i2s1: i2s@12D6 {
- status = disabled;
- };
-
- i2s2: i2s@12D7 {
- status = disabled;
- };
-
sound {
compatible = samsung,smdk-wm8994;
diff --git a/arch/arm/boot/dts/exynos5250.dtsi
b/arch/arm/boot/dts/exynos5250.dtsi
index 7d7cc77..c863113 100644
--- a/arch/arm/boot/dts/exynos5250.dtsi
+++ b/arch/arm/boot/dts/exynos5250.dtsi
@@ -417,6 +417,7 @@
i2s0: i2s@0383 {
compatible = samsung,s5pv210-i2s;
+ status = disabled;
reg = 0x0383 0x100;
dmas = pdma0 10
pdma0 9
@@ -433,6 +434,7 @@
i2s1: i2s@12D6 {
compatible = samsung,s3c6410-i2s;
+ status = disabled;
reg = 0x12D6 0x100;
dmas = pdma1 12
pdma1 11;
@@ -445,6 +447,7 @@
i2s2: i2s@12D7 {
compatible = samsung,s3c6410-i2s;
+ status = disabled;
reg = 0x12D7 0x100;
dmas = pdma0 12
pdma0 11;
--
1.8.4.rc3
--
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
[PATCH] drm/exynos: fix return value check in lowlevel_buffer_allocate()
From: Wei Yongjun yongjun_...@trendmicro.com.cn
In case of error, the function drm_prime_pages_to_sg() returns ERR_PTR()
and never returns NULL. The NULL test in the return value check should
be replaced with IS_ERR().
Signed-off-by: Wei Yongjun yongjun_...@trendmicro.com.cn
---
drivers/gpu/drm/exynos/exynos_drm_buf.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_buf.c
b/drivers/gpu/drm/exynos/exynos_drm_buf.c
index 3445a0f..e3ee833 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_buf.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_buf.c
@@ -90,9 +90,9 @@ static int lowlevel_buffer_allocate(struct drm_device *dev,
}
buf-sgt = drm_prime_pages_to_sg(buf-pages, nr_pages);
- if (!buf-sgt) {
+ if (IS_ERR(buf-sgt)) {
DRM_ERROR(failed to get sg table.\n);
- ret = -ENOMEM;
+ ret = PTR_ERR(buf-sgt);
goto err_free_attrs;
}
--
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: [RFC Patch v2 0/3] add temporary parent migration support
On 4 September 2013 23:31, Tomasz Figa tomasz.f...@gmail.com wrote: On Wednesday 04 of September 2013 10:43:28 Mike Turquette wrote: Quoting Tomasz Figa (2013-09-03 15:36:50) Hi Chander, On Tuesday 03 of September 2013 17:04:28 Chander Kashyap wrote: Some platform has provision to change cpu parent clock during cpu frequency scaling. This patch series provides a mechanism to implement the same using CCF. Patch1 provides mechanism to migrate to new parent temporarily. Patch2 updates the user of clk_register_mux and DEFINE_CLK_MUX which are modified to add support for clk migration. Patch3 adds support to Exynos5250 to use the clock parent migration feature implemented in CCF. I don't really like this approach. A need to change mux setting temporarily is heavily platform-specific and I don't think it should be handled by generic code. I agree with Tomasz. First of all there are many factor that you would have to account for to make this solution generic, such as: - board specific alternative parents, - exact moment of parent change, - some other platform specific conditions, like CPU voltage that must be changed when mux is changed, because it changes CPU frequency, - and probably a lot of more factors that only people working with all the platforms supported (and unsupported yet) by Linux. I can see at least two solutions for this problem that don't require changing core code of common clock framework: 1) Implementing a special clock type using normal mux ops, but also registering a notifier for its PRE_RATE_CHANGE and POST_RATE_CHANGE events to perform parent switching. Creating a custom clock type is the way to go here. It is possible to wrap the mux clk_ops to re-use that code, or just write a custom clock type from scratch. I do not like using the clock rate-change notifiers for this purpose. The notifiers provide hooks to drivers that need to take care around clock transitions. Using the notifiers from within the clock framework indicates poor design. I was not sure how a .set_parent() from inside a .set_rate() would interact with rate setting, so I mentioned notifiers here, but now as I think of it, CCF is supposed to be re-entrant, so things should be fine. 2) Using normal mux clock, but registering such notifiers in clock controller or cpufreq driver. This depends on what the data sheet or reference manual states. If using a temporary parent is a property of the clock programming sequence (e.g. to have a glitch-less transition) then that logic belongs in the clock provider driver (i.e. a custom clock type needs to be created with this logic). However if using a temporary parent is not required for programming the clock, but is instead a requirement of the clock consumer (e.g. a CPU, or some I/O controller) then perhaps putting this logic in that driver is the right way to go. In that case the logic could be explicit: clk_set_parent(clk, temp_parent); clk_set_rate(clk, target_rate); clk_set_parent(clk, target_parent); Or it could implicit with the use of rate-change notifiers. Again the rate-change notifiers exist for clock consumer drivers to use, so this is OK. I have a hunch that the right way to do this is for a custom clock type to be created which simply calls clk_set_parent from within the clock's .set_rate callback, but I'll wait on feedback from Chander on the needs of his platform. I believe Chander has exactly the same use case for this as we have for Exynos 4210 and 4x12. Yes Tomasz, I am trying to address the same issue. As this was requirement for more than one platform so thought of making it generic. On these SoCs, CPU frequency is being scaled by reconfiguring PLL, which needs to be masked for locking time. To let the CPU operate normally, a mux that allows switching CPU clock domain between two PLLs can be switched to the other PLL (MPLL) until the main ARM PLL (APLL) starts operating again. However this issue is not limited to clocks, because there must be also a voltage transition involved if the utility PLL has frequency higher than the one currently being reconfigured. Best regards, Tomasz -- with warm regards, Chander Kashyap -- 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: [RFC Patch v2 0/3] add temporary parent migration support
On 6 September 2013 00:02, Mike Turquette mturque...@linaro.org wrote: On Wed, Sep 4, 2013 at 11:01 AM, Tomasz Figa tomasz.f...@gmail.com wrote: On Wednesday 04 of September 2013 10:43:28 Mike Turquette wrote: Quoting Tomasz Figa (2013-09-03 15:36:50) Hi Chander, On Tuesday 03 of September 2013 17:04:28 Chander Kashyap wrote: Some platform has provision to change cpu parent clock during cpu frequency scaling. This patch series provides a mechanism to implement the same using CCF. Patch1 provides mechanism to migrate to new parent temporarily. Patch2 updates the user of clk_register_mux and DEFINE_CLK_MUX which are modified to add support for clk migration. Patch3 adds support to Exynos5250 to use the clock parent migration feature implemented in CCF. I don't really like this approach. A need to change mux setting temporarily is heavily platform-specific and I don't think it should be handled by generic code. I agree with Tomasz. First of all there are many factor that you would have to account for to make this solution generic, such as: - board specific alternative parents, - exact moment of parent change, - some other platform specific conditions, like CPU voltage that must be changed when mux is changed, because it changes CPU frequency, - and probably a lot of more factors that only people working with all the platforms supported (and unsupported yet) by Linux. I can see at least two solutions for this problem that don't require changing core code of common clock framework: 1) Implementing a special clock type using normal mux ops, but also registering a notifier for its PRE_RATE_CHANGE and POST_RATE_CHANGE events to perform parent switching. Creating a custom clock type is the way to go here. It is possible to wrap the mux clk_ops to re-use that code, or just write a custom clock type from scratch. I do not like using the clock rate-change notifiers for this purpose. The notifiers provide hooks to drivers that need to take care around clock transitions. Using the notifiers from within the clock framework indicates poor design. I was not sure how a .set_parent() from inside a .set_rate() would interact with rate setting, so I mentioned notifiers here, but now as I think of it, CCF is supposed to be re-entrant, so things should be fine. 2) Using normal mux clock, but registering such notifiers in clock controller or cpufreq driver. This depends on what the data sheet or reference manual states. If using a temporary parent is a property of the clock programming sequence (e.g. to have a glitch-less transition) then that logic belongs in the clock provider driver (i.e. a custom clock type needs to be created with this logic). However if using a temporary parent is not required for programming the clock, but is instead a requirement of the clock consumer (e.g. a CPU, or some I/O controller) then perhaps putting this logic in that driver is the right way to go. In that case the logic could be explicit: clk_set_parent(clk, temp_parent); clk_set_rate(clk, target_rate); clk_set_parent(clk, target_parent); Or it could implicit with the use of rate-change notifiers. Again the rate-change notifiers exist for clock consumer drivers to use, so this is OK. I have a hunch that the right way to do this is for a custom clock type to be created which simply calls clk_set_parent from within the clock's .set_rate callback, but I'll wait on feedback from Chander on the needs of his platform. I believe Chander has exactly the same use case for this as we have for Exynos 4210 and 4x12. On these SoCs, CPU frequency is being scaled by reconfiguring PLL, which needs to be masked for locking time. To let the CPU operate normally, a mux that allows switching CPU clock domain between two PLLs can be switched to the other PLL (MPLL) until the main ARM PLL (APLL) starts operating again. Right, this is the glitchless operation I mentioned earlier and it is not uncommon in PLLs. However this issue is not limited to clocks, because there must be also a voltage transition involved if the utility PLL has frequency higher than the one currently being reconfigured. Right, that is an altogether different issue. What I would like to see is the temporary parent managed by calls to clk_set_parent from within a custom .set_rate callback. As for the voltage scaling, it would be cool to see this working with the voltage notifier series I posted recently. Both parents of the PLLs can have their own operating point tables and each transition would fire off notifiers that scale voltage. Something like this: clk_set_rate(pll, rate) - enter .set_rate - clk_set_parent(pll, temp_parent) - temp_parent PRE_RATE_CHANGE notifier is triggered, maybe scale voltage - .set_parent callback - temp_parent POST_RATE_CHANGE notifier is triggered, maybe scale
Re: [PATCH v7 13/13] V4L: Add driver for s5k4e5 image sensor
Hi Sylwester, On Fri, Sep 6, 2013 at 1:32 AM, Sylwester Nawrocki sylvester.nawro...@gmail.com wrote: On 08/21/2013 08:34 AM, Arun Kumar K wrote: This patch adds subdev driver for Samsung S5K4E5 raw image sensor. Like s5k6a3, it is also another fimc-is firmware controlled sensor. This minimal sensor driver doesn't do any I2C communications as its done by ISP firmware. It can be updated if needed to a regular sensor driver by adding the I2C communication. Signed-off-by: Arun Kumar Karun...@samsung.com Reviewed-by: Sylwester Nawrockis.nawro...@samsung.com --- .../devicetree/bindings/media/i2c/s5k4e5.txt | 43 +++ drivers/media/i2c/Kconfig |8 + drivers/media/i2c/Makefile |1 + drivers/media/i2c/s5k4e5.c | 361 4 files changed, 413 insertions(+) create mode 100644 Documentation/devicetree/bindings/media/i2c/s5k4e5.txt create mode 100644 drivers/media/i2c/s5k4e5.c diff --git a/Documentation/devicetree/bindings/media/i2c/s5k4e5.txt b/Documentation/devicetree/bindings/media/i2c/s5k4e5.txt new file mode 100644 index 000..5af462c --- /dev/null +++ b/Documentation/devicetree/bindings/media/i2c/s5k4e5.txt @@ -0,0 +1,43 @@ +* Samsung S5K4E5 Raw Image Sensor + +S5K4E5 is a raw image sensor with maximum resolution of 2560x1920 +pixels. Data transfer is carried out via MIPI CSI-2 port and controls +via I2C bus. + +Required Properties: +- compatible : must be samsung,s5k4e5 +- reg : I2C device address +- gpios: reset gpio pin I guess this should be reset-gpios. How about changing description to: - reset-gpios : specifier of a GPIO connected to the RESET pin; ? If I name it to reset-gpios, the function of_get_gpio_flags() in the driver fails. This function searches for the entry with name gpios. Is it still recommended to use a custom name and parse it explicitly? Regards Arun -- 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
