Re: [PATCH v4 07/10] exynos4-is: Add clock provider for the SCLK_CAM clock outputs

2014-02-22 Thread Sylwester Nawrocki 

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-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 v4 07/10] exynos4-is: Add clock provider for the SCLK_CAM clock outputs

2014-02-21 Thread Mark Rutland 
On Thu, Feb 20, 2014 at 07:40:34PM +, Sylwester Nawrocki wrote:
 This patch adds clock provider so the the SCLK_CAM0/1 output clocks
 can be accessed by image sensor devices through the clk API.
 
 Signed-off-by: Sylwester Nawrocki s.nawro...@samsung.com
 Acked-by: Kyungmin Park kyungmin.p...@samsung.com
 ---
 Changes since v3:
  - use clock-output-names DT property instead of hard coding names
of registered clocks in the driver; first two entries of the
clock-names property value are used to specify parent clocks of
cam_{a,b}_clkout clocks, rather than hard coding it to sclk_cam{0,1}
in the driver.
  - addressed issues pointed out in review by Mauro.
 
 Changes since v2:
  - use 'camera' DT node drirectly as clock provider node, rather than
   creating additional clock-controller child node.
  - DT binding documentation moved to a separate patch.
 ---
  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?

 + 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?

 + 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?

The binding and commit message(s) implied only two clocks were, so
what's the point in exporting clocks which aren't available?

Thanks,
Mark.
--
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 v4 07/10] exynos4-is: Add clock provider for the SCLK_CAM clock outputs

2014-02-20 Thread Sylwester Nawrocki 
This patch adds clock provider so the the SCLK_CAM0/1 output clocks
can be accessed by image sensor devices through the clk API.

Signed-off-by: Sylwester Nawrocki s.nawro...@samsung.com
Acked-by: Kyungmin Park kyungmin.p...@samsung.com
---
Changes since v3:
 - use clock-output-names DT property instead of hard coding names
   of registered clocks in the driver; first two entries of the
   clock-names property value are used to specify parent clocks of
   cam_{a,b}_clkout clocks, rather than hard coding it to sclk_cam{0,1}
   in the driver.
 - addressed issues pointed out in review by Mauro.

Changes since v2:
 - use 'camera' DT node drirectly as clock provider node, rather than
  creating additional clock-controller child node.
 - DT binding documentation moved to a separate patch.
---
 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(-)

diff --git a/drivers/media/platform/exynos4-is/media-dev.c 
b/drivers/media/platform/exynos4-is/media-dev.c
index c1bce17..dfc3131 100644
--- a/drivers/media/platform/exynos4-is/media-dev.c
+++ b/drivers/media/platform/exynos4-is/media-dev.c
@@ -11,6 +11,8 @@
  */
 
 #include linux/bug.h
+#include linux/clk.h
+#include linux/clk-provider.h
 #include linux/device.h
 #include linux/errno.h
 #include linux/i2c.h
@@ -1437,6 +1439,103 @@ static int fimc_md_get_pinctrl(struct fimc_md *fmd)
return 0;
 }
 
+#ifdef CONFIG_OF
+static int cam_clk_prepare(struct clk_hw *hw)
+{
+   struct cam_clk *camclk = to_cam_clk(hw);
+   int ret;
+
+   if (camclk-fmd-pmf == NULL)
+   return -ENODEV;
+
+   ret = pm_runtime_get_sync(camclk-fmd-pmf);
+   return ret  0 ? ret : 0;
+}
+
+static void cam_clk_unprepare(struct clk_hw *hw)
+{
+   struct cam_clk *camclk = to_cam_clk(hw);
+
+   if (camclk-fmd-pmf == NULL)
+   return;
+
+   pm_runtime_put_sync(camclk-fmd-pmf);
+}
+
+static const struct clk_ops cam_clk_ops = {
+   .prepare = cam_clk_prepare,
+   .unprepare = cam_clk_unprepare,
+};
+
+static void fimc_md_unregister_clk_provider(struct fimc_md *fmd)
+{
+   struct cam_clk_provider *cp = fmd-clk_provider;
+   unsigned int i;
+
+   if (cp-of_node)
+   of_clk_del_provider(cp-of_node);
+
+   for (i = 0; i  cp-num_clocks; i++)
+   clk_unregister(cp-clks[i]);
+}
+
+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);
+   if (ret  0)
+   break;
+
+   ret = of_property_read_string_index(dev-of_node,
+   clock-names, i, init.parent_names);
+   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);
+   if (ret == 0)
+   return 0;
+err:
+   fimc_md_unregister_clk_provider(fmd);
+   return ret;
+}
+#else
+#define fimc_md_register_clk_provider(fmd) (0)
+#define fimc_md_unregister_clk_provider(fmd) (0)
+#endif
+
 static int fimc_md_probe(struct platform_device *pdev)
 {
struct device *dev = pdev-dev;
@@ -1464,16 +1563,24 @@ static int fimc_md_probe(struct platform_device *pdev)
 
fmd-use_isp = fimc_md_is_isp_available(dev-of_node);
 
+   ret = fimc_md_register_clk_provider(fmd);
+   if (ret  0) {
+   v4l2_err(v4l2_dev, clock provider registration failed\n);
+   return ret;
+   }
+
ret = v4l2_device_register(dev, fmd-v4l2_dev);
if (ret  0) {
v4l2_err(v4l2_dev, Failed to register v4l2_device: %d\n, ret);