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=-5.3 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable 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 E3D86C54FD3 for ; Wed, 25 Mar 2020 14:35:36 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id B2E0020777 for ; Wed, 25 Mar 2020 14:35:36 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="u6luavn1" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727569AbgCYOff (ORCPT ); Wed, 25 Mar 2020 10:35:35 -0400 Received: from mail-qv1-f68.google.com ([209.85.219.68]:32895 "EHLO mail-qv1-f68.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727505AbgCYOff (ORCPT ); Wed, 25 Mar 2020 10:35:35 -0400 Received: by mail-qv1-f68.google.com with SMTP id p19so1149372qve.0 for ; Wed, 25 Mar 2020 07:35:34 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=+kwXOGpMaPekrAzfhftfE+gw1YnXEMbIx547SEtRJs8=; b=u6luavn1Vsw0nblaWrGICZrBOI5aygYZDNJfSeSFwROrkidjCDpGRJgdi+mMUs+U9A 26JsvmnurOMQEScqA26HQRFi4gm1TdEoiPfyNgm61+5ElwYvpXdwjZIq0WDJNpo6GOIl mRyvDRy0cvxi/aMelDBdZxLuBFv3lg9MAHWR5F940+ht5bkDBC7PRRUzuPqTHg+mP+r2 xX1yUBj2Sp6HbxoRaFEK7zBrX7C44/pI6m1V8rQbsm8ojLa5IKBI/k9HD5JNpH62a9TS b491lAKXeaksvjjDBsDn77sAoj/E6wd/c03D8zTphOR3/5K9c6XkVzfgh/qzgpk4UXGy T5FA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=+kwXOGpMaPekrAzfhftfE+gw1YnXEMbIx547SEtRJs8=; b=jQ20BtxznR5HfZyMNm3A+/acH7LU2W/gs+CDRPo1in+FzdjY7Ai+2umLcCqceqTSB2 xt5hHUbCddu0TQAzoV42eUP2xhGeTIIqLAaj5YrwKtAyxw9ZfvIgustxNfMzf1DgQndz efgLIB7nESGrCpBWwYVMB7yuF+nMdrTjEFcRCnHs39l5khwmxSVigBnsGoB06rY8H+s5 p8vXWVa/aep7bmucdvku5J+768ld85yOHaVr5GYgPstYTxmJ6otGl65EWopHgufY69mz 5r5xEwukJbiQVHHFQ87QLsPRl3u7sQgUOHlbBJLRYBt0Bqxiq7ipepYBQpMKedqtgttg fUwA== X-Gm-Message-State: ANhLgQ3b0YKXLB/jB2x/ADers5zww2VKQ/rvR8m/ZVNGGYW4Vi9uXPaY P2Jww/eQyFF9+UX1iIQADb8k7Q== X-Google-Smtp-Source: ADFU+vuNxobH1Mq6LBSt0Der6JSj8ojaHpucegR+t8b4DvSDA49x5wkorDVhZyM7KlZMcxClhmiQkg== X-Received: by 2002:a0c:fe87:: with SMTP id d7mr2362960qvs.37.1585146934157; Wed, 25 Mar 2020 07:35:34 -0700 (PDT) Received: from [192.168.1.92] (pool-71-255-246-27.washdc.fios.verizon.net. [71.255.246.27]) by smtp.gmail.com with ESMTPSA id l18sm15345100qke.132.2020.03.25.07.35.32 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 25 Mar 2020 07:35:33 -0700 (PDT) Subject: Re: [Patch v5 3/6] thermal: Add generic power domain warming device driver. To: Ulf Hansson Cc: Zhang Rui , Daniel Lezcano , Bjorn Andersson , Andy Gross , Rob Herring , Amit Kucheria , Mark Rutland , "Rafael J. Wysocki" , Linux PM , DTML , linux-arm-msm , Linux Kernel Mailing List References: <20200320014107.26087-1-thara.gopinath@linaro.org> <20200320014107.26087-4-thara.gopinath@linaro.org> From: Thara Gopinath Message-ID: <31aba776-28ee-3aac-08eb-6f39f8279bfe@linaro.org> Date: Wed, 25 Mar 2020 10:35:31 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.4.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-arm-msm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-arm-msm@vger.kernel.org Hi Ulf, Thanks for the review! On 3/23/20 11:57 AM, Ulf Hansson wrote: --snip >> + >> +static void pd_warming_release(struct device *dev) >> +{ >> + kfree(dev); > > This is wrong, you should free a "struct pd_warming_device *". Use the > "container of" macro to get it from 'dev'. Will fix this. > >> +} >> + >> +struct thermal_cooling_device * >> +of_pd_warming_register(struct device *parent, int pd_id) >> +{ >> + struct pd_warming_device *pd_wdev; >> + struct of_phandle_args pd_args; >> + char cdev_name[THERMAL_NAME_LENGTH]; >> + int ret; >> + >> + pd_wdev = kzalloc(sizeof(*pd_wdev), GFP_KERNEL); >> + if (!pd_wdev) >> + return ERR_PTR(-ENOMEM); >> + >> + dev_set_name(&pd_wdev->dev, "%s_%d_warming_dev", >> + dev_name(parent), pd_id); >> + pd_wdev->dev.parent = parent; >> + pd_wdev->dev.release = pd_warming_release; >> + >> + ret = device_register(&pd_wdev->dev); >> + if (ret) { >> + put_device(&pd_wdev->dev); >> + goto free_pd_wdev; >> + } >> + >> + ret = ida_simple_get(&pd_ida, 0, 0, GFP_KERNEL); >> + if (ret < 0) >> + goto unregister_device; > > If you use and ida, you might as well use it as a part of the > dev_set_name() above. > > That should give you a unique name, similar to how you use it for the > cdev_name below. dev_set_name above already has a unique name with the power controller name and the power domain id. cdev on the other hand creates a virtual thermal device and needs a unique name. > >> + >> + pd_wdev->id = ret; >> + >> + pd_args.np = parent->of_node; >> + pd_args.args[0] = pd_id; >> + pd_args.args_count = 1; >> + >> + ret = of_genpd_add_device(&pd_args, &pd_wdev->dev); >> + >> + if (ret) >> + goto remove_ida; >> + >> + ret = dev_pm_genpd_performance_state_count(&pd_wdev->dev); >> + if (ret < 0) >> + goto out_genpd; >> + >> + pd_wdev->max_state = ret - 1; >> + pm_runtime_enable(&pd_wdev->dev); >> + pd_wdev->runtime_resumed = false; >> + >> + snprintf(cdev_name, sizeof(cdev_name), "thermal-pd-%d", pd_wdev->id); >> + pd_wdev->cdev = thermal_of_cooling_device_register >> + (NULL, cdev_name, pd_wdev, >> + &pd_warming_device_ops); >> + if (IS_ERR(pd_wdev->cdev)) { >> + pr_err("unable to register %s cooling device\n", cdev_name); >> + ret = PTR_ERR(pd_wdev->cdev); >> + goto out_runtime_disable; >> + } >> + >> + return pd_wdev->cdev; >> + >> +out_runtime_disable: >> + pm_runtime_disable(&pd_wdev->dev); >> +out_genpd: >> + pm_genpd_remove_device(&pd_wdev->dev); >> +remove_ida: >> + ida_simple_remove(&pd_ida, pd_wdev->id); >> +unregister_device: >> + device_unregister(&pd_wdev->dev); >> + pd_warming_release(&pd_wdev->dev); > > This is wrong, drop this. Oops . sorry . Will do. Will fix rest of the comments below as well. > >> +free_pd_wdev: >> + kfree(pd_wdev); > > Since you should free this from the ->release() callback, there is no > need to do this here. > >> + return ERR_PTR(ret); >> +} >> +EXPORT_SYMBOL_GPL(of_pd_warming_register); >> + >> +void pd_warming_unregister(struct thermal_cooling_device *cdev) >> +{ >> + struct pd_warming_device *pd_wdev = cdev->devdata; >> + struct device *dev = &pd_wdev->dev; >> + >> + if (pd_wdev->runtime_resumed) { >> + dev_pm_genpd_set_performance_state(dev, 0); >> + pm_runtime_put(dev); >> + pd_wdev->runtime_resumed = false; >> + } >> + pm_runtime_disable(dev); >> + pm_genpd_remove_device(dev); >> + ida_simple_remove(&pd_ida, pd_wdev->id); >> + thermal_cooling_device_unregister(cdev); >> + kfree(pd_wdev); > > Don't use kfree here, but instead device_unregister(dev); > >> +} >> +EXPORT_SYMBOL_GPL(pd_warming_unregister); >> diff --git a/include/linux/pd_warming.h b/include/linux/pd_warming.h >> new file mode 100644 >> index 000000000000..550a5683b56d >> --- /dev/null >> +++ b/include/linux/pd_warming.h >> @@ -0,0 +1,29 @@ >> +// SPDX-License-Identifier: GPL-2.0 >> +/* >> + * Copyright (c) 2019, Linaro Ltd. >> + */ >> +#ifndef __PWR_DOMAIN_WARMING_H__ >> +#define __PWR_DOMAIN_WARMING_H__ >> + >> +#include >> +#include >> + >> +#ifdef CONFIG_PWR_DOMAIN_WARMING_THERMAL >> +struct thermal_cooling_device * >> +of_pd_warming_register(struct device *parent, int pd_id); >> + >> +void pd_warming_unregister(struct thermal_cooling_device *cdev); >> + >> +#else >> +static inline struct thermal_cooling_device * >> +of_pd_warming_register(struct device *parent, int pd_id) >> +{ >> + return ERR_PTR(-ENOSYS); >> +} >> + >> +static inline void >> +pd_warming_unregister(struct thermal_cooling_device *cdev) >> +{ >> +} >> +#endif /* CONFIG_PWR_DOMAIN_WARMING_THERMAL */ >> +#endif /* __PWR_DOMAIN_WARMING_H__ */ >> -- >> 2.20.1 >> > > Besides the few things above, this looks good to me. > > Kind regards > Uffe > -- Warm Regards Thara