From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-7.1 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id F0945C7618B for ; Fri, 26 Jul 2019 10:46:00 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id B242D22BEF for ; Fri, 26 Jul 2019 10:46:00 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1564137960; bh=x9mpS5NvmpKmAahIcVZmBhUi4LVxeCyzdFwA6MTZsMU=; h=References:In-Reply-To:From:Date:Subject:To:Cc:List-ID:From; b=VJwimIrFXraFXLYfqdb675WGfCPfppfhCuMIWEwXpFHTK9ZGbCPuIfpyqRlMw4TiC QinCntuPmj+DGNKWFHSNaLuR7gWLUo3ipcjoTroE7Nt9ri+ea72b2djbauM2dJHAyG 5Quuur3Mt4XlFkLXQQkgmVaVQQFigHYRg704wpLg= Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726591AbfGZKqA (ORCPT ); Fri, 26 Jul 2019 06:46:00 -0400 Received: from mail.kernel.org ([198.145.29.99]:54224 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726203AbfGZKqA (ORCPT ); Fri, 26 Jul 2019 06:46:00 -0400 Received: from mail-lj1-f177.google.com (mail-lj1-f177.google.com [209.85.208.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id C574F22CBB; Fri, 26 Jul 2019 10:45:57 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1564137958; bh=x9mpS5NvmpKmAahIcVZmBhUi4LVxeCyzdFwA6MTZsMU=; h=References:In-Reply-To:From:Date:Subject:To:Cc:From; b=q5TngikPRTXMNg0jozDNBJPwFVF/esEnumTmy+r7ILHTK8YXiAtYXiv4n0Ftz/gYH VoWYJrImvalMWzFplysthaI8Pd2QGW14KO/MIM/bQ9BOcY6m+ViYxbnLc9ibtDq8W2 YWBb3du4rBxkznulmmTs9BFTIKNfvEuXon0wj6fQ= Received: by mail-lj1-f177.google.com with SMTP id h10so51050503ljg.0; Fri, 26 Jul 2019 03:45:57 -0700 (PDT) X-Gm-Message-State: APjAAAXIO3bswoo3QQIAVLl8SdZkcRbdIjtUDfg5fezTqIKVGDB0dcgF MqyRjtCZ14lsc6rfoHKe3xgRQ9ljLjjTDWVmgNc= X-Google-Smtp-Source: APXvYqzZkUIlkzQtInKbgDs9Nar+L2o6NkagjMqtMfPToLfoMzoo3NIJHIWVhDdfUt4SL85oQ13GjXQLdyM04b3g+uU= X-Received: by 2002:a2e:8155:: with SMTP id t21mr48690964ljg.80.1564137955920; Fri, 26 Jul 2019 03:45:55 -0700 (PDT) MIME-Version: 1.0 References: <20190723122016.30279-1-a.swigon@partner.samsung.com> <20190723122016.30279-5-a.swigon@partner.samsung.com> In-Reply-To: From: Krzysztof Kozlowski Date: Fri, 26 Jul 2019 12:45:44 +0200 X-Gmail-Original-Message-ID: Message-ID: Subject: Re: [RFC PATCH 04/11] devfreq: exynos-bus: Clean up code To: cwchoi00@gmail.com Cc: =?UTF-8?B?QXJ0dXIgxZp3aWdvxYQ=?= , devicetree , linux-arm-kernel , linux-samsung-soc , linux-kernel , Linux PM list , dri-devel , Chanwoo Choi , MyungJoo Ham , Inki Dae , Seung-Woo Kim , georgi.djakov@linaro.org, Marek Szyprowski Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Sender: linux-pm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pm@vger.kernel.org On Thu, 25 Jul 2019 at 14:51, Chanwoo Choi wrote: > > 2019=EB=85=84 7=EC=9B=94 24=EC=9D=BC (=EC=88=98) =EC=98=A4=EC=A0=84 8:07,= Artur =C5=9Awigo=C5=84 =EB=8B=98=EC=9D=B4 = =EC=9E=91=EC=84=B1: > > > > This patch adds minor improvements to the exynos-bus driver. > > > > Signed-off-by: Artur =C5=9Awigo=C5=84 > > --- > > drivers/devfreq/exynos-bus.c | 49 ++++++++++++++++-------------------- > > 1 file changed, 22 insertions(+), 27 deletions(-) > > > > diff --git a/drivers/devfreq/exynos-bus.c b/drivers/devfreq/exynos-bus.= c > > index 4bb83b945bf7..412511ca7703 100644 > > --- a/drivers/devfreq/exynos-bus.c > > +++ b/drivers/devfreq/exynos-bus.c > > @@ -15,11 +15,10 @@ > > #include > > #include > > #include > > -#include > > +#include > > #include > > #include > > #include > > -#include > > > > #define DEFAULT_SATURATION_RATIO 40 > > #define DEFAULT_VOLTAGE_TOLERANCE 2 > > @@ -256,7 +255,7 @@ static int exynos_bus_parent_parse_of(struct device= _node *np, > > struct exynos_bus *bus) > > { > > struct device *dev =3D bus->dev; > > - int i, ret, count, size; > > + int i, ret, count; > > > > /* Get the regulator to provide each bus with the power */ > > bus->regulator =3D devm_regulator_get(dev, "vdd"); > > @@ -283,8 +282,7 @@ static int exynos_bus_parent_parse_of(struct device= _node *np, > > } > > bus->edev_count =3D count; > > > > - size =3D sizeof(*bus->edev) * count; > > - bus->edev =3D devm_kzalloc(dev, size, GFP_KERNEL); > > + bus->edev =3D devm_kcalloc(dev, count, sizeof(*bus->edev), GFP_= KERNEL); > > if (!bus->edev) { > > ret =3D -ENOMEM; > > goto err_regulator; > > @@ -388,7 +386,7 @@ static int exynos_bus_profile_init(struct exynos_bu= s *bus, > > ondemand_data =3D devm_kzalloc(dev, sizeof(*ondemand_data), GFP= _KERNEL); > > if (!ondemand_data) { > > ret =3D -ENOMEM; > > - goto err; > > + goto out; > > } > > ondemand_data->upthreshold =3D 40; > > ondemand_data->downdifferential =3D 5; > > @@ -400,14 +398,14 @@ static int exynos_bus_profile_init(struct exynos_= bus *bus, > > if (IS_ERR(bus->devfreq)) { > > dev_err(dev, "failed to add devfreq device\n"); > > ret =3D PTR_ERR(bus->devfreq); > > - goto err; > > + goto out; > > } > > > > /* Register opp_notifier to catch the change of OPP */ > > ret =3D devm_devfreq_register_opp_notifier(dev, bus->devfreq); > > if (ret < 0) { > > dev_err(dev, "failed to register opp notifier\n"); > > - goto err; > > + goto out; > > } > > > > /* > > @@ -417,16 +415,16 @@ static int exynos_bus_profile_init(struct exynos_= bus *bus, > > ret =3D exynos_bus_enable_edev(bus); > > if (ret < 0) { > > dev_err(dev, "failed to enable devfreq-event devices\n"= ); > > - goto err; > > + goto out; > > } > > > > ret =3D exynos_bus_set_event(bus); > > if (ret < 0) { > > dev_err(dev, "failed to set event to devfreq-event devi= ces\n"); > > - goto err; > > + goto out; > > } > > > > -err: > > +out: > > return ret; > > } > > > > @@ -446,27 +444,28 @@ static int exynos_bus_profile_init_passive(struct= exynos_bus *bus, > > parent_devfreq =3D devfreq_get_devfreq_by_phandle(dev, 0); > > if (IS_ERR(parent_devfreq)) { > > ret =3D -EPROBE_DEFER; > > - goto err; > > + goto out; > > } > > > > passive_data =3D devm_kzalloc(dev, sizeof(*passive_data), GFP_K= ERNEL); > > if (!passive_data) { > > ret =3D -ENOMEM; > > - goto err; > > + goto out; > > } > > passive_data->parent =3D parent_devfreq; > > > > /* Add devfreq device for exynos bus with passive governor */ > > - bus->devfreq =3D devm_devfreq_add_device(dev, profile, DEVFREQ_= GOV_PASSIVE, > > + bus->devfreq =3D devm_devfreq_add_device(dev, profile, > > + DEVFREQ_GOV_PASSIVE, > > passive_data); > > if (IS_ERR(bus->devfreq)) { > > dev_err(dev, > > "failed to add devfreq dev with passive governo= r\n"); > > ret =3D PTR_ERR(bus->devfreq); > > - goto err; > > + goto out; > > } > > > > -err: > > +out: > > return ret; > > } > > > > @@ -484,11 +483,11 @@ static int exynos_bus_probe(struct platform_devic= e *pdev) > > return -EINVAL; > > } > > > > - bus =3D devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL); > > + bus =3D devm_kzalloc(dev, sizeof(*bus), GFP_KERNEL); > > if (!bus) > > return -ENOMEM; > > mutex_init(&bus->lock); > > - bus->dev =3D &pdev->dev; > > + bus->dev =3D dev; > > platform_set_drvdata(pdev, bus); > > > > /* Parse the device-tree to get the resource information */ > > @@ -502,7 +501,7 @@ static int exynos_bus_probe(struct platform_device = *pdev) > > goto err; > > } > > > > - node =3D of_parse_phandle(dev->of_node, "devfreq", 0); > > + node =3D of_parse_phandle(np, "devfreq", 0); > > if (node) { > > of_node_put(node); > > ret =3D exynos_bus_profile_init_passive(bus, profile); > > @@ -547,12 +546,10 @@ static int exynos_bus_resume(struct device *dev) > > int ret; > > > > ret =3D exynos_bus_enable_edev(bus); > > - if (ret < 0) { > > + if (ret < 0) > > dev_err(dev, "failed to enable the devfreq-event device= s\n"); > > - return ret; > > - } > > > > - return 0; > > + return ret; > > } > > > > static int exynos_bus_suspend(struct device *dev) > > @@ -561,12 +558,10 @@ static int exynos_bus_suspend(struct device *dev) > > int ret; > > > > NACK. Instead of simple nack you should explain what is wrong with proposed approach. The existing code could be improved and this patch clearly improves the readability. "err" label is confusing if it is used for correct exit path. The last "if() return" block is subjective - but then explain exactly why you prefer one over another. No just "nack" for peoples contributions. > > ret =3D exynos_bus_disable_edev(bus); > > - if (ret < 0) { > > + if (ret < 0) > > dev_err(dev, "failed to disable the devfreq-event devic= es\n"); > > - return ret; > > - } > > > > - return 0; > > + return ret; > > } > > #endif > > > > -- > > 2.17.1 > > > > NACK. > > As I already commented, It has never any benefit. > I think that it is not usful. Please drop it. The benefit for me is clear - makes the code easier to understand. Best regards, Krzysztof