Re: [PATCH v4 03/10] Documentation: devicetree: Update Samsung FIMC DT binding
On 02/21/2014 04:50 PM, Mark Rutland wrote: On Thu, Feb 20, 2014 at 07:40:30PM +, Sylwester Nawrocki wrote: This patch documents following updates of the Exynos4 SoC camera subsystem devicetree binding: - addition of #clock-cells property to 'camera' node - the #clock-cells property is needed when the sensor sub-devices use clock provided by the camera host interface; - addition of an optional clock-output-names property; - change of the clock-frequency at image sensor node from mandatory to an optional property - there should be no need to require this property by the camera host device binding, a default frequency value can ofen be used; - addition of a requirement of specific order of values in clocks/ clock-names properties, so the first two entry in the clock-names property can be used as parent clock names for the camera master clock provider. It happens all in-kernel dts files list the clock in such order, thus there should be no regression as far as in-kernel dts files are concerned. I'm not sure I follow the reasoning here. Why does this matter? Why can child nodes not get these by name if they have to? These input clocks won't be used directly by child nodes. More details below... Signed-off-by: Sylwester Nawrockis.nawro...@samsung.com Acked-by: Kyungmin Parkkyungmin.p...@samsung.com --- .../devicetree/bindings/media/samsung-fimc.txt | 36 +++- 1 file changed, 28 insertions(+), 8 deletions(-) diff --git a/Documentation/devicetree/bindings/media/samsung-fimc.txt b/Documentation/devicetree/bindings/media/samsung-fimc.txt index 96312f6..1a5820d 100644 --- a/Documentation/devicetree/bindings/media/samsung-fimc.txt +++ b/Documentation/devicetree/bindings/media/samsung-fimc.txt @@ -20,6 +20,7 @@ Required properties: the clock-names property; - clock-names : must contain sclk_cam0, sclk_cam1, pxl_async0, pxl_async1 entries, matching entries in the clocks property. + First two entries must be sclk_cam0, sclk_cam1. I don't think this is a good idea. The pinctrl bindings defined in ../pinctrl/pinctrl-bindings.txt must be used to define a required pinctrl state named default and optional pinctrl states: @@ -32,6 +33,22 @@ way around. The 'camera' node must include at least one 'fimc' child node. +Optional properties (*: Is that a smiley face? Sorry, it was supposed to be a reference mark, will remove this. +- #clock-cells: from the common clock bindings (../clock/clock-bindings.txt), + must be 1. A clock provider is associated with the 'camera' node and it should + be referenced by external sensors that use clocks provided by the SoC on + CAM_*_CLKOUT pins. The clock specifier cell stores an index of a clock. + The indices are 0, 1 for CAM_A_CLKOUT, CAM_B_CLKOUT clocks respectively. + +- clock-output-names: from the common clock bindings, should contain names of + clocks registered by the camera subsystem corresponding to CAM_A_CLKOUT, + CAM_B_CLKOUT output clocks, in this order. Parent clock of these clocks are + specified be first two entries of the clock-names property. Do you need this? All right, that might have been a bad idea, it mixes names of clocks registered by the main clock controller with names of clock input lines at the device. It's a mistake I have been usually sensitive to and now made it myself. :/ My intention was to maintain the clock tree, since the camera block doesn't generate the clock itself, it merely passes through the clocks from the SoC main clock controller (CMU). So clk parents need to be properly set and since there is no clock-output-names property at the CMU DT node, of_clk_get_parent_name() cannot be used. So presumably the DT binding would be only specifying that the sclk_cam0, sclk_cam1 clock input entries are associated with output clocks named as in clock-output-names property. And the driver could either: 1) hard code those (well defined) CMU clock (clk parent) names, 2) clk_get() its input clock, retrieve name with __clk_get_name() and pass it as parent name to clk_register() - it sounds a bit hacky though. The output clock names could be also well defined by the binding per the IP's compatible. Nevertheless using clock-output-names seems cleaner to me than defining character strings in the driver. What do you think ? That's not how clock-names is supposed to work. The clock-names property is for the names of the _input_ clock lines on the device, not the output names on whichever parent clock they came from. Yes, I agree, that was a pretty bad abuse of the bindings. Any clock-names property description should define absolutely the set of names. As this does not, NAK. + +(* #clock-cells and clock-output-names are mandatory properties if external +image sensor devices reference 'camera' device node as a clock provider. s/(*/Note:/ Thanks, will fix that.
Re: [PATCH v4 07/10] exynos4-is: Add clock provider for the SCLK_CAM clock outputs
On 02/21/2014 05:05 PM, Mark Rutland wrote:
---
drivers/media/platform/exynos4-is/media-dev.c | 110 +
drivers/media/platform/exynos4-is/media-dev.h | 19 -
2 files changed, 128 insertions(+), 1 deletion(-)
...
+static int fimc_md_register_clk_provider(struct fimc_md *fmd)
+{
+ struct cam_clk_provider *cp =fmd-clk_provider;
+ struct device *dev =fmd-pdev-dev;
+ int i, ret;
+
+ for (i = 0; i ARRAY_SIZE(cp-clks); i++) {
+ struct cam_clk *camclk =cp-camclk[i];
+ struct clk_init_data init;
+
+ ret = of_property_read_string_index(dev-of_node,
+ clock-output-names, i,init.name);
Are there not well-defined names for the clock outputs of the block?
There are, but this driver handles multiple SoCs so I thought it's better
to define these names in devicetree. They are supposed to be unique. Isn't
that what the clock-output-names property is for ?
+ if (ret 0)
+ break;
+
+ ret = of_property_read_string_index(dev-of_node,
+ clock-names, i, init.parent_names);
This shouldn't be a parent name. It should be the input line name.
I don't think this makes sense.
Why do you need the name of the parent clock?
As explained in previous e-mail, it is needed to maintain the clock tree,
the parent clock names must be set properly. As the above is not correct
I could hard code these names or retrieve with __clk_get_name() from proper
input clocks. of_clk_get_parent_name() cannot be used.
+ if (ret 0)
+ break;
+
+ init.num_parents = 1;
+ init.ops =cam_clk_ops;
+ init.flags = CLK_SET_RATE_PARENT;
+ camclk-hw.init =init;
+ camclk-fmd = fmd;
+
+ cp-clks[i] = clk_register(NULL,camclk-hw);
+ if (IS_ERR(cp-clks[i])) {
+ dev_err(dev, failed to register clock: %s (%ld)\n,
+ init.name, PTR_ERR(cp-clks[i]));
+ ret = PTR_ERR(cp-clks[i]);
+ goto err;
+ }
+ cp-num_clocks++;
+ }
+
+ if (cp-num_clocks == 0) {
+ dev_warn(dev, clk provider not registered\n);
+ return 0;
+ }
+
+ cp-clk_data.clks = cp-clks;
+ cp-clk_data.clk_num = cp-num_clocks;
+ cp-of_node = dev-of_node;
+ ret = of_clk_add_provider(dev-of_node, of_clk_src_onecell_get,
+ cp-clk_data);
Are _all_ of the input clock lines available to children in hardware?
This code is only for two clocks, ARRAY_SIZE(cp-clks) == 2 and
cp-num_clocks can't be greater than 2. Nevertheless, for some SoC
variants this driver handles (e.g. S5PV210) there can be only up to
two clocks listed in the camera node.
The camera interface IP block doesn't generate the clock itself, it just
passes it through when is active, i.e. its other clocks are enabled and
the power domain activated.
The binding and commit message(s) implied only two clocks were, so
what's the point in exporting clocks which aren't available?
I'm not sure what makes you think there is more than two ?
--
Regards,
Sylwester
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 03/10] Documentation: devicetree: Update Samsung FIMC DT binding
[Ccing Mike] On 22.02.2014 23:02, Sylwester Nawrocki wrote: On 02/21/2014 04:50 PM, Mark Rutland wrote: On Thu, Feb 20, 2014 at 07:40:30PM +, Sylwester Nawrocki wrote: +- #clock-cells: from the common clock bindings (../clock/clock-bindings.txt), + must be 1. A clock provider is associated with the 'camera' node and it should + be referenced by external sensors that use clocks provided by the SoC on + CAM_*_CLKOUT pins. The clock specifier cell stores an index of a clock. + The indices are 0, 1 for CAM_A_CLKOUT, CAM_B_CLKOUT clocks respectively. + +- clock-output-names: from the common clock bindings, should contain names of + clocks registered by the camera subsystem corresponding to CAM_A_CLKOUT, + CAM_B_CLKOUT output clocks, in this order. Parent clock of these clocks are + specified be first two entries of the clock-names property. Do you need this? All right, that might have been a bad idea, it mixes names of clocks registered by the main clock controller with names of clock input lines at the device. It's a mistake I have been usually sensitive to and now made it myself. :/ My intention was to maintain the clock tree, since the camera block doesn't generate the clock itself, it merely passes through the clocks from the SoC main clock controller (CMU). So clk parents need to be properly set and since there is no clock-output-names property at the CMU DT node, of_clk_get_parent_name() cannot be used. So presumably the DT binding would be only specifying that the sclk_cam0, sclk_cam1 clock input entries are associated with output clocks named as in clock-output-names property. And the driver could either: 1) hard code those (well defined) CMU clock (clk parent) names, I don't think this would be a good idea, as those CMU clock names may vary between SoCs. 2) clk_get() its input clock, retrieve name with __clk_get_name() and pass it as parent name to clk_register() - it sounds a bit hacky though. This looks fine, at least until proper interface is added to CCF. Exynos audio subsystem clock driver does exactly the same. However, the right thing would be to make it possible to use pointers to struct clk instead of strings to list parent(s). This could be done by adding .parents field (struct clk **) to clk_init_data struct and make clk_register() use it if available. The output clock names could be also well defined by the binding per the IP's compatible. Nevertheless using clock-output-names seems cleaner to me than defining character strings in the driver. What do you think ? RANT Personally, I don't like clock-output-names at all. The idea of having a global namespace for all clock providers looks flawed to me. This property only tries to work around the not-quite-right design by giving device tree the right to force CCF to use particular internal clock identifiers and avoid collisions if two providers happen to have the same internal clock names. I believe there should be completely no need for clock-output-names, (other than a textual label for debugging purposes, analogically to regulator-name). If one clock provider needs a clock from another, then you should list it using clock bindings in node of the former, not rely on some static name assignments. Then, after eliminating the need to use static names anymore, the namespace could be split into per-provider namespaces and we would have no collision issue anymore. Of course it's probably to late already for such changes, as there are already systems relying on clock-output-names (i.e. without inter-IP clock dependencies listed using clock bindings) and we would have to keep backwards compatibility anyway... /RANT Best regards, Tomasz -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 03/10] Documentation: devicetree: Update Samsung FIMC DT binding
On 02/22/2014 11:26 PM, Tomasz Figa wrote: [Ccing Mike] On 22.02.2014 23:02, Sylwester Nawrocki wrote: On 02/21/2014 04:50 PM, Mark Rutland wrote: On Thu, Feb 20, 2014 at 07:40:30PM +, Sylwester Nawrocki wrote: +- #clock-cells: from the common clock bindings (../clock/clock-bindings.txt), + must be 1. A clock provider is associated with the 'camera' node and it should + be referenced by external sensors that use clocks provided by the SoC on + CAM_*_CLKOUT pins. The clock specifier cell stores an index of a clock. + The indices are 0, 1 for CAM_A_CLKOUT, CAM_B_CLKOUT clocks respectively. + +- clock-output-names: from the common clock bindings, should contain names of + clocks registered by the camera subsystem corresponding to CAM_A_CLKOUT, + CAM_B_CLKOUT output clocks, in this order. Parent clock of these clocks are + specified be first two entries of the clock-names property. Do you need this? All right, that might have been a bad idea, it mixes names of clocks registered by the main clock controller with names of clock input lines at the device. It's a mistake I have been usually sensitive to and now made it myself. :/ My intention was to maintain the clock tree, since the camera block doesn't generate the clock itself, it merely passes through the clocks from the SoC main clock controller (CMU). So clk parents need to be properly set and since there is no clock-output-names property at the CMU DT node, of_clk_get_parent_name() cannot be used. So presumably the DT binding would be only specifying that the sclk_cam0, sclk_cam1 clock input entries are associated with output clocks named as in clock-output-names property. And the driver could either: 1) hard code those (well defined) CMU clock (clk parent) names, I don't think this would be a good idea, as those CMU clock names may vary between SoCs. For the record, I'm not in favour of this approach, even though these clock names happen to be same for all relevant SoCs. 2) clk_get() its input clock, retrieve name with __clk_get_name() and pass it as parent name to clk_register() - it sounds a bit hacky though. This looks fine, at least until proper interface is added to CCF. Exynos audio subsystem clock driver does exactly the same. Hmm, I looked at this driver and somehow missed that. However, the right thing would be to make it possible to use pointers to struct clk instead of strings to list parent(s). This could be done by adding .parents field (struct clk **) to clk_init_data struct and make clk_register() use it if available. We would just need to ensure the locking is done properly. -- Regards, Sylwester -- 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
cron job: media_tree daily build: WARNINGS
This message is generated daily by a cron job that builds media_tree for the kernels and architectures in the list below. Results of the daily build of media_tree: date: Sun Feb 23 04:00:17 CET 2014 git branch: test git hash: 37e59f876bc710d67a30b660826a5e83e07101ce gcc version:i686-linux-gcc (GCC) 4.8.2 sparse version: 0.4.5-rc1 host hardware: x86_64 host os:3.12-6.slh.2-amd64 linux-git-arm-at91: OK linux-git-arm-davinci: OK linux-git-arm-exynos: WARNINGS linux-git-arm-mx: OK linux-git-arm-omap: OK linux-git-arm-omap1: OK linux-git-arm-pxa: OK linux-git-blackfin: OK linux-git-i686: OK linux-git-m32r: OK linux-git-mips: OK linux-git-powerpc64: OK linux-git-sh: OK linux-git-x86_64: OK linux-2.6.31.14-i686: WARNINGS linux-2.6.32.27-i686: OK linux-2.6.33.7-i686: OK linux-2.6.34.7-i686: OK linux-2.6.35.9-i686: OK linux-2.6.36.4-i686: OK linux-2.6.37.6-i686: OK linux-2.6.38.8-i686: OK linux-2.6.39.4-i686: OK linux-3.0.60-i686: OK linux-3.1.10-i686: OK linux-3.2.37-i686: OK linux-3.3.8-i686: OK linux-3.4.27-i686: OK linux-3.5.7-i686: OK linux-3.6.11-i686: OK linux-3.7.4-i686: OK linux-3.8-i686: OK linux-3.9.2-i686: OK linux-3.10.1-i686: OK linux-3.11.1-i686: OK linux-3.12-i686: OK linux-3.13-i686: OK linux-3.14-rc1-i686: OK linux-2.6.31.14-x86_64: OK linux-2.6.32.27-x86_64: OK linux-2.6.33.7-x86_64: OK linux-2.6.34.7-x86_64: OK linux-2.6.35.9-x86_64: OK linux-2.6.36.4-x86_64: OK linux-2.6.37.6-x86_64: OK linux-2.6.38.8-x86_64: OK linux-2.6.39.4-x86_64: OK linux-3.0.60-x86_64: OK linux-3.1.10-x86_64: OK linux-3.2.37-x86_64: OK linux-3.3.8-x86_64: OK linux-3.4.27-x86_64: OK linux-3.5.7-x86_64: OK linux-3.6.11-x86_64: OK linux-3.7.4-x86_64: OK linux-3.8-x86_64: OK linux-3.9.2-x86_64: OK linux-3.10.1-x86_64: OK linux-3.11.1-x86_64: OK linux-3.12-x86_64: OK linux-3.13-x86_64: OK linux-3.14-rc1-x86_64: OK apps: OK spec-git: OK sparse version: 0.4.5-rc1 sparse: ERRORS Detailed results are available here: http://www.xs4all.nl/~hverkuil/logs/Sunday.log Full logs are available here: http://www.xs4all.nl/~hverkuil/logs/Sunday.tar.bz2 The Media Infrastructure API from this daily build is here: http://www.xs4all.nl/~hverkuil/spec/media.html -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv2 0/2] uvcvideo: Support Logitech RGB Bayer formats
PATCHv1 discussion: http://sourceforge.net/mailarchive/message.php?msg_id=31541036 and http://article.gmane.org/gmane.linux.drivers.video-input-infrastructure/70192 On Sun, Oct 20, 2013 at 11:54:10AM +0200, Oleksij Rempel wrote [..] Hello Peter, Laurent, Peter do have time to update this patches for latest kernel? Laurent would you accept them? Here are is updated pachset. They have been tested against uvcvideo-next today (3.14.0-rc) using FFmpeg's V4L2 grabber. Peter Ross (2): videodev2: add V4L2_PIX_FMT_S10_1X10 formats uvcvideo: Support Logitech RGB Bayer formats drivers/media/usb/uvc/uvc_driver.c | 176 + drivers/media/usb/uvc/uvc_video.c | 48 ++ drivers/media/usb/uvc/uvcvideo.h | 22 + include/uapi/linux/videodev2.h | 4 + 4 files changed, 250 insertions(+) -- 1.8.3.2 -- Peter (A907 E02F A6E5 0CD2 34CD 20D2 6760 79C5 AC40 DD6B) signature.asc Description: Digital signature
