Hi Sylwester,
Thank you for the review.
Will address all your review comments.
Some responses below:
[snip]
>> +
>> +static int fimc_md_register_sensor_entities(struct fimc_md *fmd)
>> +{
>> + struct device_node *of_node = fmd->pdev->dev.of_node;
>> + int ret;
>> +
>> + /*
>> + * Runtime resume one of the FIMC entities to make sure
>> + * the sclk_cam clocks are not globally disabled.
>
>
> It's a bit mysterious to me, is this requirement still valid on Exynos5 ?
> I glanced over the Exynos5250 datasheet and there seem to be no sclk_cam?
> clocks dependency on any of GScaler clocks. Maybe you don't need a clock
> provider in this driver, perhaps sensor drivers could use sclk_cam clocks
> directly, assigned through dts ?
>
Yes these clocks can be directly exposed via dt.
I will drop clock provider from this driver.
[snip]
>> +/*
>> + * The peripheral sensor clock management.
>> + */
>> +static void fimc_md_put_clocks(struct fimc_md *fmd)
>> +{
>> + int i = FIMC_MAX_CAMCLKS;
>> +
>> + while (--i>= 0) {
>> + if (IS_ERR(fmd->camclk[i].clock))
>> + continue;
>> + clk_put(fmd->camclk[i].clock);
>> + fmd->camclk[i].clock = ERR_PTR(-EINVAL);
>> + }
>
>
> Please double check if you need this sclk_cam clocks handling. We could
> simply add a requirement that this driver supports only sensor subdevs
> through the v4l2-async API and which controls their clock themselves.
>
sclk_cam* handling can be removed and be done from respective
sensors. But I think the sclk_bayer handling needs to be retained in the
media driver.
>> +}
>> +
>> +static int fimc_md_get_clocks(struct fimc_md *fmd)
>> +{
>> + struct device *dev = NULL;
>> + char clk_name[32];
>> + struct clk *clock;
>> + int i, ret = 0;
>> +
>> + for (i = 0; i< FIMC_MAX_CAMCLKS; i++)
>> + fmd->camclk[i].clock = ERR_PTR(-EINVAL);
>> +
>> + if (fmd->pdev->dev.of_node)
>> + dev =&fmd->pdev->dev;
>> +
>> + for (i = 0; i< SCLK_BAYER; i++) {
>> + snprintf(clk_name, sizeof(clk_name), "sclk_cam%u", i);
>> + clock = clk_get(dev, clk_name);
>> +
>> + if (IS_ERR(clock)) {
>> + dev_err(&fmd->pdev->dev, "Failed to get clock:
>> %s\n",
>> + clk_name);
>> + ret = PTR_ERR(clock);
>> + break;
>> + }
>> + fmd->camclk[i].clock = clock;
>> + }
>> + if (ret)
>> + fimc_md_put_clocks(fmd);
>> +
>> + /* Prepare bayer clk */
>> + clock = clk_get(dev, "sclk_bayer");
>> +
>> + if (IS_ERR(clock)) {
>> + dev_err(&fmd->pdev->dev, "Failed to get clock: %s\n",
>> + clk_name);
>
>
> Wrong error message.
>
>> + ret = PTR_ERR(clock);
>> + goto err_exit;
>> + }
>> + ret = clk_prepare(clock);
>> + if (ret< 0) {
>> + clk_put(clock);
>> + fmd->camclk[SCLK_BAYER].clock = ERR_PTR(-EINVAL);
>> + goto err_exit;
>> + }
>> + fmd->camclk[SCLK_BAYER].clock = clock;
>
>
> Could you explain a bit how is this SCLK_BAYER clock used ? Is it routed
> to external image sensor, or is it used only inside an SoC ?
>
It is not defined properly in the manual, but I suppose its the bus clock
for the bayer rgb data bus. So for proper sensor functionality, we need this
sclk_bayer in addition to the external sensor clks (sclk_cam*). Isn't
exynos5 media driver is the best place to handle such clocks?
>> + return 0;
>> +err_exit:
>> + fimc_md_put_clocks(fmd);
>> + return ret;
>> +}
>> +
Regards
Arun
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
