Received: by 2002:a25:e7d8:0:0:0:0:0 with SMTP id e207csp1824520ybh; Fri, 13 Mar 2020 08:04:22 -0700 (PDT) X-Google-Smtp-Source: ADFU+vstx7yHA0KK5TqGa8afBJMqXBVZ8LFVhWZcdVOQF82nVZDTNEQIcI8wqsm5vjMyfK7YfcUZ X-Received: by 2002:aca:d68e:: with SMTP id n136mr7470248oig.103.1584111862089; Fri, 13 Mar 2020 08:04:22 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1584111862; cv=none; d=google.com; s=arc-20160816; b=xvB62MYx1tCmqxJwRiphjSriG9N9OJRoOsu0yVUFPunhTplfTVt3G3JTBHGGVIDFOz zHtdEJw4Lo47pThTjtH/yPRDpWekhz6CeSpBEFKwYaefV9RJavAogVrz/33HyYEdFFsd StnZZ8OjtEyLdxfK+/zz/hd/A9Py2bYdQXUpUWRjETH+oGU5ZLks5fGtJBMi7+6Q+jMy blx23fAGaHHCnneRqmvwJtzL4p63wmFXaHQssKwuG20czr7+/2SGsLoPg9OYXwRwRw6k FuOnFIQldJ0IFH1Dokg5mNtFO44vuXPBP9Dw24fWWuSFnNpbbPC+B5319goZJKMxH5PY cXZA== 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=eFNXLKStv2hg6vAmjDX147WvWuSgMNwcWrKlrLIyNJU=; b=Qt2i7FhRJtUrllmyVlqYcc9Y75JWvqK8dHfvnQWnpnlCptQpkJ3uCIPiqhydGg3pEd afK+eVeo/TrQ5ADD/pzdrCrxFDmQNjZ5Tv1ho6CUR6ndAIssiiWjmRpJBK+sJUJlrCTi khg1rO5doJxRy9Gn8J750kS+i+rQ133KmwoYcShlLeZAQgOwgqX1egDztaO+n56/Cy9p +nNVOlqyFlW498WxsgKXvRiEMw5Jm33mkGINOgP/yd8NxBjcO/WRpCEU1FiDLh6ellb4 Cxri2pvwlPHIsZIUehM8hmNpNP6Zvv314vVGf2nsC6tnsG76SwDxOpT56fLgcdInctvu VssA== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=J7UnUbl9; 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 7si4663754oij.97.2020.03.13.08.03.38; Fri, 13 Mar 2020 08:04:22 -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=J7UnUbl9; 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 S1726664AbgCMPDE (ORCPT + 99 others); Fri, 13 Mar 2020 11:03:04 -0400 Received: from mail-qk1-f194.google.com ([209.85.222.194]:36547 "EHLO mail-qk1-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726446AbgCMPDE (ORCPT ); Fri, 13 Mar 2020 11:03:04 -0400 Received: by mail-qk1-f194.google.com with SMTP id u25so12998778qkk.3 for ; Fri, 13 Mar 2020 08:03:03 -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=eFNXLKStv2hg6vAmjDX147WvWuSgMNwcWrKlrLIyNJU=; b=J7UnUbl9ctijkZvETuhF2hc0OMJeFQjsVukcnWBIivUe52Pao9vewDvONPsWAupqgx UM0886uf9mLGKQkU8VQB185wQS4WwmPkjWc7UHdAyV94aSuWySlHmE3FGcedek/oV3U8 HKuw+IesrCpz8oNHIrjywlWJk1IgKlgmy9pBlwC+sm1avY33ghWFsISK6hhcW5V5s6Yh O0ZYROVcAEk9yUOiEisjBzVgwaDiWEj+s148o4pQ5oseaCe6ZhdmWpYGEpRcRIc2b5W7 VjBfh6G3zbf2sZkbnpjF2exFmkcBBJpqPStIdF/dPabjVf+fPz/sESjzTqqWyafLKhzT UeGA== 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=eFNXLKStv2hg6vAmjDX147WvWuSgMNwcWrKlrLIyNJU=; b=aCud6op19ZhJNVrsXGLgcJbFQ60ffJjgODxxHcOVPmFmIhkpSo8T+eKu9tnDCsUoJl M9S8FmhAkaTT7WlAeUp1t7pCa6VDeZXNS9QD+pNZ4/W10LZ6q/6qyg+KxkkmeKGsP+5O IaB6bQ1y1BRCEeH762IwQ/5bhV3FvkoaMhWk67kjfpLXJjZRZ2UKcsBYbq8qGAShRufy vFdYIp0VAjQlQVlaVI7v5TJ432K//p1sYJj1rEpzPlSrLxIuSxx3t2SMzJayz2DPFQo6 JiRRn/fCbZEAfa7oo7gvkQQTb9bFPeYU019NVSnPap0UdHA87natXf7qGb5dVmGkaq+W YwHg== X-Gm-Message-State: ANhLgQ1UIT5JgSMHgK6VHGy6TSFfJrrrLBhvEh1i6MffM+DRQNNh7b07 XvUggw6LvATG4t8CW9mNeTadCMSSH8E= X-Received: by 2002:a37:7d2:: with SMTP id 201mr13919655qkh.146.1584111781830; Fri, 13 Mar 2020 08:03:01 -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 c190sm8142895qkb.80.2020.03.13.08.02.59 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 13 Mar 2020 08:03:00 -0700 (PDT) Subject: Re: [Patch v4 4/7] thermal: Add generic power domain warming device driver. To: Ulf Hansson Cc: Eduardo Valentin , Zhang Rui , Daniel Lezcano , Bjorn Andersson , Andy Gross , Amit Kucheria , Mark Rutland , "Rafael J. Wysocki" , Linux PM , DTML , linux-arm-msm , Linux Kernel Mailing List References: <1574254593-16078-1-git-send-email-thara.gopinath@linaro.org> <1574254593-16078-5-git-send-email-thara.gopinath@linaro.org> From: Thara Gopinath Message-ID: Date: Fri, 13 Mar 2020 11:02:58 -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 reviews. Will send out v5 soon. On 3/13/20 9:13 AM, Ulf Hansson wrote: > [...] > >>>> +static struct thermal_cooling_device_ops pd_warming_device_ops = { >>>> + .get_max_state = ::pd_wdev_get_max_state, >>>> + .get_cur_state = pd_wdev_get_cur_state, >>>> + .set_cur_state = pd_wdev_set_cur_state, >>>> +}; >>>> + >>>> +struct thermal_cooling_device * >>>> +pwr_domain_warming_register(struct device *parent, char *pd_name, int pd_id) >>> >>> Maybe rename this to: thermal_of_pd_warming_register() >> >> How about pd_of_warming_register? It is consistent with other cooling >> device register like cpuidle_of_cooling_register and >> cpufreq_of_cooling_register. > > Well, we actually have the following: > of_devfreq_cooling_register() > of_cpufreq_cooling_register() > cpuidle_of_cooling_register() > > So maybe this is the most consistent. :-) > of_pd_warming_register() Sure! > >> >>> Moreover, I think you could replace the "struct device *parent", with >>> a "struct device_node *node" as in-parameter. That's all you need, >>> right? >> >> You mean pd_wdev->dev.parent need not be populated ? The device >> in this case will be created under /sys/devices which I do not think >> is the correct. > > Good point! > >> With a parent device specified, the power controller that resides the >> power domain that can act as the warming dev, becomes the parent of the >> warming dev. In case of this patch series, for the mx warming dev, >> 179c0000.rsc/179c0000.rsc\:power-controller/ becomes the parent.(The >> device will be created under >> /sys/devices/platform/soc\@0/179c0000.rsc/179c0000.rsc\:power-controller/) >> >> Other way might be to register the warming device under virtual devices >> as a new category of devices. > > No, that sounds wrong. > > Another option is to create a specific bus type for these new > pd_warming devices. But I admit that sounds a bit too much, let's > assign a parent. > >> >> I prefer to keep it as a child of the power controller device, but I am >> open to explore other options and to re-do this bit. What do you think? > > Sure, sorry for the noise. No issues! > >> >>> >>>> +{ >>>> + struct pd_warming_device *pd_wdev; >>>> + struct of_phandle_args pd_args; >>>> + int ret; >>>> + >>>> + pd_wdev = kzalloc(sizeof(*pd_wdev), GFP_KERNEL); >>>> + if (!pd_wdev) >>>> + return ERR_PTR(-ENOMEM); >>>> + >>>> + dev_set_name(&pd_wdev->dev, "%s_warming_dev", pd_name); >>> >>> Perhaps skip the in-param *pd_name and make use of the suggested >>> "struct device_node *node", the index and something with "warming...", >>> when setting the name. >> >> Won't the index have to be in-param in this case ? > > Isn't that already the case? > > Huh, long time since I reviewed this. > >> >>> >>> Just an idea, as to simplify for the caller. >>> >>>> + pd_wdev->dev.parent = parent; >>> >>> This isn't needed, I think. > > So ignore this comment, as discussed above. > >>> >>>> + >>>> + ret = device_register(&pd_wdev->dev); >>>> + if (ret) >>>> + goto error; >>>> + >>>> + 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); >>>> + >>> >>> White space. >> >> Will fix it. >> >>> >>>> + if (ret) >>>> + goto error; >>>> + >>>> + ret = dev_pm_genpd_performance_state_count(&pd_wdev->dev); >>>> + if (ret < 0) >>>> + goto error; >>>> + >>>> + pd_wdev->max_state = ret - 1; >>>> + pm_runtime_enable(&pd_wdev->dev); >>>> + pd_wdev->runtime_resumed = false; >>>> + >>>> + pd_wdev->cdev = thermal_of_cooling_device_parent_register >>>> + (NULL, parent, pd_name, pd_wdev, >>>> + &pd_warming_device_ops); >>> >>> As stated in patch3, I don't get it why you need to use this new API >>> for "parents". >> >> I agree with you. I cannot re-collect my thought process for this API. >> I compiled and tested using the regular API and everything works fine. >> I will drop patch 3 and use the thermal_of_cooling_device_register here. > > Great, one confusing piece seems to go away then. :-) > >> >>> >>>> + if (IS_ERR(pd_wdev->cdev)) { >>>> + pr_err("unable to register %s cooling device\n", pd_name); >>>> + pm_runtime_disable(&pd_wdev->dev); >>>> + ret = PTR_ERR(pd_wdev->cdev); >>>> + goto error; >>>> + } >>>> + >>>> + return pd_wdev->cdev; >>>> +error: >>>> + put_device(&pd_wdev->dev); >>> >>> If device_register() succeeds you need to call device_unregister(), >>> rather than put_device() as a part of the error handling. >> >> Will fix this. >> >>> >>>> + kfree(pd_wdev); >>> >>> You need a ->release() callback to manage kfree(), after you called >>> device_register(). >> >> mm?? I did not get this. What release callback? You mean for power >> controller driver to call ? > > No, this how life cycle management of devices should be implemented. > > Have a look at genpd_release_dev() - and see how that is being used > for genpd's virtual devices, that should explain more. Ah yes. I get it now. Will fix this. -- Warm Regards Thara