W dniu 05.11.2015 o 19:40, Pavel Fedin pisze:
> Hello!
>
>>> +static int decode_sromc(struct exynos_srom *srom, struct device_node *np)
>>
>> I missed that one previously: add prefix and more descriptive name, like:
>> exynos_srom_parse_child()
>
> exynos_srom_configure_bank(), is this name OK?
Yes, its OK.
>
>>> static int exynos_srom_probe(struct platform_device *pdev)
>>> {
>>> - struct device_node *np;
>>> + struct device_node *np, *child;
>>> struct exynos_srom *srom;
>>> struct device *dev = &pdev->dev;
>>> + bool error = false;
>>
>> The 'error' name is misleading - like error for entire probe which is
>> not true.
>>
>> Instead split it to separate function like:
>>
>> +static int exynos_srom_parse_children(....) {
>> + int ret = 0;
>> +
>> + for_each_child_of_node(np, child) {
>> + ret = exynos_srom_parse_child(srom, child);
>> + if (ret) {
>> + dev_err(dev,
>> + "Could not decode bank configuration for %s: %d\n",
>> + child->name, ret);
>> + break;
>> + }
>> + }
>> +
>> + return ret;
>> +}
>
> Factoring out this loop is unnecessary, because i could just 'return 0' in the loop
> instead of 'error = true'. Byt my idea is to go through all banks anyway, just in
> case, to diagnose all of them. So that the user will be able to spot and fix all
> broken banks at once, instead of doing one-by-one.
> I have renamed the variable to 'bool bad_bank_config', will this be OK?
Yes, that's also OK.
Best regards,
Krzysztof
--
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
