[PATCH v3 4/6] exynos4-is: Add clock provider for the external clocks
Sylwester Nawrocki
sylvester.nawrocki at gmail.com
Tue Jan 7 16:23:33 EST 2014
On 01/02/2014 08:58 PM, Mauro Carvalho Chehab wrote:
> Em Thu, 17 Oct 2013 20:06:49 +0200
> Sylwester Nawrocki<s.nawrocki at samsung.com> escreveu:
>
>> This patch adds clock provider to expose the sclk_cam0/1 clocks
>> for external image sensor devices.
>>
>> Signed-off-by: Sylwester Nawrocki<s.nawrocki at samsung.com>
>> Signed-off-by: Kyungmin Park<kyungmin.park at samsung.com>
>> ---
>> Changes since v2:
>> - use 'camera' DT node drirectly as clock provider node, rather than
>> creating additional clock-controller child node.
>> ---
>> .../devicetree/bindings/media/samsung-fimc.txt | 15 ++-
>> drivers/media/platform/exynos4-is/media-dev.c | 108 ++++++++++++++++++++
>> drivers/media/platform/exynos4-is/media-dev.h | 18 +++-
>> 3 files changed, 137 insertions(+), 4 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/media/samsung-fimc.txt b/Documentation/devicetree/bindings/media/samsung-fimc.txt
>> index 96312f6..968e065 100644
>> --- a/Documentation/devicetree/bindings/media/samsung-fimc.txt
>> +++ b/Documentation/devicetree/bindings/media/samsung-fimc.txt
>> @@ -32,6 +32,15 @@ way around.
>>
>> The 'camera' node must include at least one 'fimc' child node.
>>
>> +Optional properties:
>> +
>> +- #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 second cell of the clock specifier is a clock's index.
>> + The indexes are 0, 1 for CAM_A_CLKOUT, CAM_B_CLKOUT clocks respectively.
>> +
>> +
>> 'fimc' device nodes
>> -------------------
>>
>> @@ -114,7 +123,7 @@ Example:
>> vddio-supply =<...>;
>>
>> clock-frequency =<24000000>;
>> - clocks =<...>;
>> + clocks =<&camera 1>;
>> clock-names = "mclk";
>>
>> port {
>> @@ -135,7 +144,7 @@ Example:
>> vddio-supply =<...>;
>>
>> clock-frequency =<24000000>;
>> - clocks =<...>;
>> + clocks =<&camera 0>;
>> clock-names = "mclk";
>>
>> port {
>> @@ -151,8 +160,8 @@ Example:
>> compatible = "samsung,fimc", "simple-bus";
>> #address-cells =<1>;
>> #size-cells =<1>;
>> + #clock-cells =<1>;
>> status = "okay";
>> -
>> pinctrl-names = "default";
>> pinctrl-0 =<&cam_port_a_clk_active>;
>
> I didn't see the above on the patch series you sent me on your git pull
> request. Where is it?
Mauro, I've moved it into a separate patch, since it is more appropriate
as that's a change in the DT binding [1].
My plan was to send it through the devicetree tree with your Ack, but
I forgot to resend the whole series. Sorry about that.
Moreover I did this split because you are requiring an explicit ACK
from a DT maintainer for anything that is related to devicetree.
I believe it is physically impossible to have every single patch
acked by a DT maintainer. Subsystem maintainers should handle minor
stuff, otherwise we won't be able to have _anything_ merged upstream.
Pinging the DT maintainers for every single patch seems a bit silly and
I'm already fed up with it. I find this whole process frustrating and very
long times to get things merged really discourage to develop anything in
mainline.
[...]
>> +static const char *cam_clk_p_names[] = { "sclk_cam0", "sclk_cam1" };
>> +
>> +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< ARRAY_SIZE(cp->clks); i++)
>> + if (!IS_ERR(cp->clks[i]))
>> + clk_unregister(cp->clks[i]);
>
> Huh? Why to initialize an array with an error code??? Does it make sense to
> have one of the clocks with an error and the others ok, and to store the
> error code? The code below doesn't seem to allow that.
If fimc_md_unregister_clk_provider() fails at any point
fimc_md_register_clk_provider() can be called to undo what has been
registered successfully. That's the point of storing the error codes.
The error paths in probe() are already complex enough.
> Just initialize cp->clks with zero and test it with:
>
> if (cp->clks[i])
> clk_unregister(cp->clks[i]);
I'm afraid this is incorrect, clock pointers should be treated as
opaque cookies and tested only with IS_ERR(). Clk users should not
assume NULL clk pointer indicate an invalid clock.
> That makes it easier to understand and review.
Maybe it is easier to understand but it is a wrong API usage. And
I believe it should never be suggested in reviews.
>> +}
>> +
>> +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++)
>> + cp->clks[i] = ERR_PTR(-EINVAL);
>
> That looks weird for me, due to several reasons:
>
> 1) ARRAY_SIZE(cp->clks) is equal to FIMC_MAX_CAMCLKS. Why are you using
> different syntaxes on the first loop and on the next one? Just to loose
> more time from reviewers to double check what number is bigger?
The loop limit argument is valid, I can correct that so the code is
easier to follow.
> 2) Why don't you just do:
>
> memset(cp->clks, ARRAY_SIZE(cp->clks), 0).
>
> or initialize struct fimc_md with kzalloc()?
Because it isn't correct. struct fimc_md is already allocated with
kzalloc(), I would certainly consider the above simpler code if it
were valid.
>> +
>> + for (i = 0; i< FIMC_MAX_CAMCLKS; i++) {
>> + struct cam_clk *camclk =&cp->camclk[i];
>> + struct clk_init_data init;
>> + char clk_name[16];
>> + struct clk *clk;
>> +
>> + snprintf(clk_name, sizeof(clk_name), "cam_clkout%d", i);
>> +
>> + init.name = clk_name;
>> + init.ops =&cam_clk_ops;
>> + init.flags = CLK_SET_RATE_PARENT;
>> + init.parent_names =&cam_clk_p_names[i];
>> + init.num_parents = 1;
>> + camclk->hw.init =&init;
>> + camclk->fmd = fmd;
>> +
>> + clk = clk_register(dev,&camclk->hw);
>> + if (IS_ERR(clk)) {
>> + dev_err(dev, "failed to register clock: %s (%ld)\n",
>> + clk_name, PTR_ERR(clk));
>> + ret = PTR_ERR(clk);
>> + goto err;
>> + }
>> + cp->clks[i] = clk;
>> + }
>> +
>> + cp->clk_data.clks = cp->clks;
>> + cp->clk_data.clk_num = i;
>> + cp->of_node = dev->of_node;
>> +
>> + ret = of_clk_add_provider(dev->of_node, of_clk_src_onecell_get,
>> + &cp->clk_data);
>> + if (!ret)
>> + 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
--
Regards,
Sylwester
[1]
http://git.linuxtv.org/snawrocki/samsung.git/shortlog/refs/heads/v3.14-exynos4-is-dt
More information about the linux-arm-kernel
mailing list