From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1032040AbbKEKke (ORCPT ); Thu, 5 Nov 2015 05:40:34 -0500 Received: from mailout2.w1.samsung.com ([210.118.77.12]:44994 "EHLO mailout2.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1031086AbbKEKka (ORCPT ); Thu, 5 Nov 2015 05:40:30 -0500 X-AuditID: cbfec7f4-f79c56d0000012ee-0a-563b321b86ed From: Pavel Fedin To: "'Krzysztof Kozlowski'" , devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-samsung-soc@vger.kernel.org, linux-kernel@vger.kernel.org Cc: "'Rob Herring'" , "'Pawel Moll'" , "'Mark Rutland'" , "'Ian Campbell'" , "'Kumar Gala'" , "'Kukjin Kim'" References: <72fa025c634fa477b1d7a89c254bd3c9c43f2b27.1446542020.git.p.fedin@samsung.com> <5639BFD8.6040006@samsung.com> In-reply-to: <5639BFD8.6040006@samsung.com> Subject: RE: [PATCH v5 3/4] drivers: exynos-srom: Add support for bank configuration Date: Thu, 05 Nov 2015 13:40:26 +0300 Message-id: <004001d117b6$62baa1a0$282fe4e0$@samsung.com> MIME-version: 1.0 Content-type: text/plain; charset=us-ascii Content-transfer-encoding: 7bit X-Mailer: Microsoft Outlook 14.0 Thread-index: AQInd0NHxwReN7LBFLXzkPqpXKHCoQGBdGTJAU6VTUidyfyvAA== Content-language: ru X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFprHIsWRmVeSWpSXmKPExsVy+t/xa7rSRtZhBnP2WFrMP3KO1aL/zUJW i3OvVjJavH5haNH/+DWzxabH11gtLu+aw2Yx4/w+Joul1y8yWUyYvpbFonXvEXYHbo8189Yw elzu62XyWLn8C5vHplWdbB6bl9R79G1ZxejxeZNcAHsUl01Kak5mWWqRvl0CV8aubwtYC85y V6w9cpy1gfE1RxcjJ4eEgInEzb6HzBC2mMSFe+vZuhi5OIQEljJKzN2xgxHC+c4oMb/zJAtI FZuAusTprx9YQBIiArsYJX5t6QZzmAXeMEqcWvaYGaJlOaPE3qYmsBZOAW2Jt7d+sncxcnAI C4RJHHmZBhJmEVCV+NvbzQ5i8wpYSjQ9uMQGYQtK/Jh8D6yVWUBLYv3O40wQtrzE5jVvoW5V kNhx9jUjiC0i4CQxedVWqHoRiWn/7jFPYBSahWTULCSjZiEZNQtJywJGllWMoqmlyQXFSem5 hnrFibnFpXnpesn5uZsYIbH2ZQfj4mNWhxgFOBiVeHgNqq3ChFgTy4orcw8xSnAwK4nwSsla hwnxpiRWVqUW5ccXleakFh9ilOZgURLnnbvrfYiQQHpiSWp2ampBahFMlomDU6qBcVY566uI HSHZ7l+yHQNeXfh95yL3D+OW5PRcjgM8CruZ7/Qfl1saKKVb6nD58ea3LYELghozUj48vbGE b8utO0xJd84lhs1f+Hvblh9l3Do1d27+qTr9sHGRdp5rlITjYa5zxTzhPl8LnDtr1do4bwrM sv0+a5bI09Kst6tPbYlMf9F2rSTruBJLcUaioRZzUXEiAF+xvemxAgAA Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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? > > 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? Kind regards, Pavel Fedin Expert Engineer Samsung Electronics Research center Russia