Hi Sachin,
Thank you for the review.
Will address your comments in next iteration.
Regards
Arun
On Thu, Jun 6, 2013 at 10:50 AM, Sachin Kamat <sachin.kamat@xxxxxxxxxx> wrote:
> Hi Arun,
>
> On 31 May 2013 18:33, Arun Kumar K <arun.kk@xxxxxxxxxxx> wrote:
>> This driver is for the FIMC-IS IP available in Samsung Exynos5
>> SoC onwards. This patch adds the core files for the new driver.
>>
>> Signed-off-by: Arun Kumar K <arun.kk@xxxxxxxxxxx>
>> Signed-off-by: Kilyeon Im <kilyeon.im@xxxxxxxxxxx>
>> ---
> [snip]
>
>> +
>> +static void fimc_is_clk_put(struct fimc_is *is)
>> +{
>> + int i;
>> +
>> + for (i = 0; i < IS_CLK_MAX_NUM; i++) {
>> + if (IS_ERR_OR_NULL(is->clock[i]))
>
> You should not check for NULL here. Instead initialize the clocks to
> some error value (like "is->clock[i] = ERR_PTR(-EINVAL);" )
> and use IS_ERR only.
>
>> + continue;
>> + clk_unprepare(is->clock[i]);
>> + clk_put(is->clock[i]);
>> + is->clock[i] = NULL;
>> + }
>> +}
>> +
>> +static int fimc_is_clk_get(struct fimc_is *is)
>> +{
>> + struct device *dev = &is->pdev->dev;
>> + int i, ret;
>> +
>> + for (i = 0; i < IS_CLK_MAX_NUM; i++) {
>> + is->clock[i] = clk_get(dev, fimc_is_clock_name[i]);
>> + if (IS_ERR(is->clock[i]))
>> + goto err;
>> + ret = clk_prepare(is->clock[i]);
>> + if (ret < 0) {
>> + clk_put(is->clock[i]);
>> + is->clock[i] = NULL;
>
> is->clock[i] = ERR_PTR(-EINVAL);
>
>> + goto err;
>> + }
>> + }
>> + return 0;
>> +err:
>> + fimc_is_clk_put(is);
>> + pr_err("Failed to get clock: %s\n", fimc_is_clock_name[i]);
>> + return -ENXIO;
>> +}
>> +
>> +static int fimc_is_clk_cfg(struct fimc_is *is)
>> +{
>> + int ret;
>> +
>> + ret = fimc_is_clk_get(is);
>> + if (ret)
>> + return ret;
>> +
>> + /* Set rates */
>> + ret = clk_set_rate(is->clock[IS_CLK_MCU_ISP_DIV0], 200 * 1000000);
>> + ret |= clk_set_rate(is->clock[IS_CLK_MCU_ISP_DIV1], 100 * 1000000);
>> + ret |= clk_set_rate(is->clock[IS_CLK_ISP_DIV0], 134 * 1000000);
>> + ret |= clk_set_rate(is->clock[IS_CLK_ISP_DIV1], 68 * 1000000);
>> + ret |= clk_set_rate(is->clock[IS_CLK_ISP_DIVMPWM], 34 * 1000000);
>> +
>> + if (ret)
>> + return -EINVAL;
>> +
>> + return 0;
>> +}
>> +
>> +static int fimc_is_probe(struct platform_device *pdev)
>> +{
>> + struct device *dev = &pdev->dev;
>> + struct resource res;
>> + struct fimc_is *is;
>> + struct pinctrl *pctrl;
>> + void __iomem *regs;
>> + struct device_node *node;
>> + int irq, ret;
>> +
>> + pr_debug("FIMC-IS Probe Enter\n");
>> +
>> + if (!pdev->dev.of_node)
>> + return -ENODEV;
>> +
>> + is = devm_kzalloc(&pdev->dev, sizeof(*is), GFP_KERNEL);
>> + if (!is)
>> + return -ENOMEM;
>> +
>> + is->pdev = pdev;
>> +
>> + ret = of_address_to_resource(dev->of_node, 0, &res);
>> + if (ret < 0)
>> + return ret;
>> +
>> + regs = devm_ioremap_resource(dev, &res);
>> + if (regs == NULL) {
>
> Please use if(IS_ERR(regs))
>
>> + dev_err(dev, "Failed to obtain io memory\n");
>
> This is not needed as devm_ioremap_resource prints the appropriate
> error messages.
>
>> + return -ENOENT;
>
> return PTR_ERR(regs);
>
> Don't forget to include <linux/err.h> for using PTR_ERR() .
>
> --
> With warm regards,
> Sachin
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
