Messages in this thread | | | Subject | Re: [PATCH v5 3/4] drivers: exynos-srom: Add support for bank configuration | From | Krzysztof Kozlowski <> | Date | Thu, 5 Nov 2015 20:09:29 +0900 |
| |
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
| |