Re: [PATCH 3/3] drm/exynos: Add device tree support for fimc ipp driver

[Sylwester Nawrocki <s.nawrocki@samsung.com>]

On 04/19/2013 01:38 PM, Eunchul Kim wrote:
>>   static void fimc_set_type_ctrl(struct fimc_context *ctx, enum fimc_wb wb)
>> @@ -1628,7 +1617,9 @@ static int fimc_ippdrv_start(struct device *dev, enum
>> drm_exynos_ipp_cmd cmd)
>>           fimc_handle_lastend(ctx, true);
>>
>>           /* setup FIMD */
>> -        fimc_set_camblk_fimd0_wb(ctx);
>> +        ret = fimc_set_camblk_fimd0_wb(ctx);
>> +        if (ret < 0)
> 
> - please adds error log information for indicating error.

OK, I'll add it.

>> +            return ret;
>>
>>           set_wb.enable = 1;
>>           set_wb.refresh = property->refresh_rate;
>> @@ -1784,26 +1775,50 @@ e_clk_free:
>>       return ret;
>>   }
>>
>> +static int fimc_parse_dt(struct fimc_context *ctx)
>> +{
>> +    struct device_node *node = ctx->dev->of_node;
>> +
>> +    if (!of_property_read_bool(node, "samsung,lcd-wb"))
> 
> - It also need error log ?

No, but it definitely deserves some comment why it is done like this.
Please see my other reply to Inki.

>> +        return -ENODEV;
>> +
>> +    if (of_property_read_u32(node, "clock-frequency",
>> +                    &ctx->clk_frequency))
>> +        ctx->clk_frequency = FIMC_DEFAULT_LCLK_FREQUENCY;
> 
> - We have many kind of H/W version of FIMC device. I think we need to use pdata
> instead of static value.

This is just a fallback value that will be used when "clock-frequency"
property is not found in fimc node in the device tree.

>> +    ctx->id = of_alias_get_id(node, "fimc");
>> +    if (ctx->id < 0)
>> +        return -EINVAL;
>> +
>> +    return 0;
>> +}
>> +
>>   static int fimc_probe(struct platform_device *pdev)
>>   {
>>       struct device *dev = &pdev->dev;
>>       struct fimc_context *ctx;
>>       struct resource *res;
>>       struct exynos_drm_ippdrv *ippdrv;
>> -    struct exynos_drm_fimc_pdata *pdata;
>> -    struct fimc_driverdata *ddata;
>>       int ret;
>>
>> -    pdata = pdev->dev.platform_data;
>> -    if (!pdata) {
>> -        dev_err(dev, "no platform data specified.\n");
>> -        return -EINVAL;
>> -    }
>> +    if (!dev->of_node)
> 
> - ditto.(error log)

Probably won't hurt, I'll add it.

>> +        return -ENODEV;
>>
>>       ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
>>       if (!ctx)
>>           return -ENOMEM;
>>
>> +    ctx->dev = dev;
>> +
>> +    ret = fimc_parse_dt(ctx);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    ctx->sysreg = syscon_regmap_lookup_by_phandle(dev->of_node,
>> +                        "samsung,sysreg");
>> +    if (IS_ERR(ctx->sysreg))
> 
> - ditto.(error log)

OK. Will add here as well.