Received: by 2002:a25:6193:0:0:0:0:0 with SMTP id v141csp679391ybb; Wed, 25 Mar 2020 07:36:25 -0700 (PDT) X-Google-Smtp-Source: ADFU+vv1CO1/V7TsG3TaSOOCDQdX8f/Xrfd49cvEcF+xp1+dVRJ3cd+Wfx1fgjg28DJqLPNwAdGR X-Received: by 2002:a4a:9b07:: with SMTP id a7mr1448762ook.78.1585146985498; Wed, 25 Mar 2020 07:36:25 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1585146985; cv=none; d=google.com; s=arc-20160816; b=Vuxv4gjoHk/W48Knup6pxCEnksqD89vEgv9HdnZdt7X8ap5kczlnQmUYaDAIvsnraN KhXaPOcva1Whh2dEtrRrmjjUQaU7DEII0zZ+i8pNe04E0SNzstPdRgDXqkEYOsX3x1ol mlqbYvs5psVsznCusgvLEr9I/mqlkk+mSfUCZpHB0M3UZwbdiAK8z49GNOATgFPcRcPF cnq17EHCm21W4UVhXTSa5UEFV2qgm9RS9qTM3T/M2u6e1h4QO1JdObsfchUT2KxjJOrC FJvZKGp2MP90rNcBEU3dIDgI3ticJ5j2LAJSIvyTyhodRYTZ8V7sQxmJVapsGlRT0WL4 ifXw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding :content-language:in-reply-to:mime-version:user-agent:date :message-id:from:references:cc:to:subject:dkim-signature; bh=+kwXOGpMaPekrAzfhftfE+gw1YnXEMbIx547SEtRJs8=; b=u979IyUpBjNOVAwD5U7IhTzLs8gAGZMyEuqZ7ZJg+klpPje+3rzEoYyNir4EfJ+Z0v M3s+BbBKJ4zZNy1yKciEmXTTLG6h+hWP2bXV1U63Y8ai0uxSdsA07E3DUMpSSwJteHSr x7d2CT7YDx0rSUam3/LC/EzY5sNSD+LHwgIpeNtzu/IHkH1aWh318psSVlS1HOksw1HL p6XJVafjQ5ssg9b2c4wsN++wXk9j6LKEgozm0E9XdOPrxG0PSgXU+CLiJfB2I4g0N+yB O8HFiOKpVTM9CD37NcqCJC4Cbf/fAsfMzS2K7GoUb4wXEsXqQcZhJDQX/TcffXZB63ke Y2cQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=u6luavn1; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id j18si10264040oib.191.2020.03.25.07.36.09; Wed, 25 Mar 2020 07:36:25 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=u6luavn1; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727689AbgCYOfi (ORCPT + 99 others); Wed, 25 Mar 2020 10:35:38 -0400 Received: from mail-qv1-f67.google.com ([209.85.219.67]:46554 "EHLO mail-qv1-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727849AbgCYOfh (ORCPT ); Wed, 25 Mar 2020 10:35:37 -0400 Received: by mail-qv1-f67.google.com with SMTP id m2so1113665qvu.13 for ; Wed, 25 Mar 2020 07:35:37 -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=PcwV65+P4zE3rkvO8wWunMSKjKMDgw0is54EMlWZAtJGhFlV8j8OvIZRWHkuXGwznE uHm5XDRoboZlq2D+2oxfNgrj+NgI1T15w9IlppeAZUzKDV9tb72sB931oUTcv0FbCnr8 IMoJcb/Z6bFnnoAmO2LPp95HbsbGhk8PCJAmx1fYYjLiv+23JjFQJapK1gykG7s+/dC+ vKIRidSJf/7LXRHzmMRm/wD9koGMCPTnH4D+i8J85lKy/3erDFQ7WLtiIc6/Ab7i2Tss uSSmKGDkB9gu3jc3Ac6nAa8WN0W7BhprFvwzQexObNesdg+Hrci8A2Mfyd5Zj4oA7BQa 32+w== X-Gm-Message-State: ANhLgQ2wb1QV/wCpdIBt26U+0gqbJp/iFYGMa4l9reQJvvmE+3/9CPX+ nqqZPrj7NmIDbjMgLBG0gnJf4mYAOAI= 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-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@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