Received: by 2002:a25:4158:0:0:0:0:0 with SMTP id o85csp1102335yba; Thu, 16 May 2019 14:27:53 -0700 (PDT) X-Google-Smtp-Source: APXvYqzaLOkBXVaWvrmu/lxvHswayP5D962uCvTQyFySX919sVXTmX8WJe59tLc0GfLIfR+nW1Dh X-Received: by 2002:a17:902:bcc6:: with SMTP id o6mr50735507pls.275.1558042073652; Thu, 16 May 2019 14:27:53 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1558042073; cv=none; d=google.com; s=arc-20160816; b=HG0jSwbkchWcaKjaQLsw7hDf/nOBXpmVe5Rdk+joknD7R+5T01O8F9/0Zw/slXBgHi olcgDOfFEhMPc4HclBEtYgvi2jECKXadEifKv5kl/wlVpg4tWWu/CTLTeKUTWHeLhmku 3vAMJnn1x0KXLEBl0/qI8UA4OWlT9H/ER9D9b0NKxeo5n0upb38e/mKr/odPevf0ZVZU 7MtGLn0agm+sxbFXkYQcuYeH/X0Rv1iumtZYk1vzM0AFyYMebdhAGm7b95jgsZ2DFfkz qzWqQehTvOJQFTM5NNGEpd0vgS8NJalWVYhGAvebGWID5jy9JHOkAisyR17WphRplXsb vPwQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:in-reply-to:content-transfer-encoding :content-disposition:mime-version:references:mail-followup-to :message-id:subject:cc:to:from:date:dkim-signature; bh=WdDEYlTvSYQejglYnP2urSW/0WEArIU3lMIb81JITeE=; b=YI6Ribt33rFPyQy9AJ6L/0tENAfNVCXSWuZeN3eZdZFNODx/GuwHK/SWfAy3cYt8/l q3eKF/E2QTimIClS5yWVhDfZtA5KmV1W9tGcRNSc6tloklbf6uBhTx2yEWs+9mr4Wx03 CibQg7Z2ApeUyZoyZ8EiliXY94A0VhzlfDM82vlmRMhsphjO0WGzGCyBmDdVRiZwilNG 5ej+PWW8jVdTbZVNsd9iWnZmHkncTxsjn9ZLEBYsDe/1I4c76qTcGAQe3KI6tAeR8cqA cBEMKLXb9N0a0OvvekTAEMBLNmErApA2w+m5ZTdhDpw8O2pxvvEWHDiLO8TJgmjbTJGJ w/YQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@megous.com header.s=mail header.b=DvT1vnFG; 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=REJECT sp=REJECT dis=NONE) header.from=megous.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id g16si6739273pfo.191.2019.05.16.14.27.36; Thu, 16 May 2019 14:27:53 -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=@megous.com header.s=mail header.b=DvT1vnFG; 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=REJECT sp=REJECT dis=NONE) header.from=megous.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728707AbfEPS3j (ORCPT + 99 others); Thu, 16 May 2019 14:29:39 -0400 Received: from vps.xff.cz ([195.181.215.36]:38730 "EHLO vps.xff.cz" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726357AbfEPS3j (ORCPT ); Thu, 16 May 2019 14:29:39 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=megous.com; s=mail; t=1558031376; bh=+nSXq6K8D525DqumN8hX/hXw6BuzgBYTSZ5zpX1yqMo=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=DvT1vnFG1JzohjaD6Ipxouz0agYvhmjyLA4av1glnZElsR6SKmCs24cQ3jDKBcXeY EiTHL44Sla99BoABrvWO89x5ew6seLkkze3Xo+O1l3rl+9VkiFIC86sMICKadMHXsh 3qL1H4pV+dSAgYzGYTsYIn3r79CJOpqw9CKrUunI= Date: Thu, 16 May 2019 20:29:36 +0200 From: =?utf-8?Q?Ond=C5=99ej?= Jirman To: Frank Lee Cc: rui.zhang@intel.com, Eduardo Valentin , Daniel Lezcano , robh+dt@kernel.org, Mark Rutland , Maxime Ripard , Chen-Yu Tsai , catalin.marinas@arm.com, will.deacon@arm.com, David Miller , Mauro Carvalho Chehab , Greg Kroah-Hartman , Jonathan.Cameron@huawei.com, Nicolas Ferre , paulmck@linux.ibm.com, Andy Gross , olof@lixom.net, bjorn.andersson@linaro.org, Jagan Teki , marc.w.gonzalez@free.fr, stefan.wahren@i2se.com, enric.balletbo@collabora.com, devicetree@vger.kernel.org, Linux Kernel Mailing List , Linux ARM , Linux PM Subject: Re: [PATCH 2/3] thermal: sun50i: add thermal driver for h6 Message-ID: <20190516182936.h6xdzp3gtg4ikave@core.my.home> Mail-Followup-To: Frank Lee , rui.zhang@intel.com, Eduardo Valentin , Daniel Lezcano , robh+dt@kernel.org, Mark Rutland , Maxime Ripard , Chen-Yu Tsai , catalin.marinas@arm.com, will.deacon@arm.com, David Miller , Mauro Carvalho Chehab , Greg Kroah-Hartman , Jonathan.Cameron@huawei.com, Nicolas Ferre , paulmck@linux.ibm.com, Andy Gross , olof@lixom.net, bjorn.andersson@linaro.org, Jagan Teki , marc.w.gonzalez@free.fr, stefan.wahren@i2se.com, enric.balletbo@collabora.com, devicetree@vger.kernel.org, Linux Kernel Mailing List , Linux ARM , Linux PM References: <20190512082614.9045-1-tiny.windzz@gmail.com> <20190512082614.9045-3-tiny.windzz@gmail.com> <20190512221612.ubmknvim4utnqpl4@core.my.home> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Yangtao, thank you for work on this driver. On Fri, May 17, 2019 at 02:06:53AM +0800, Frank Lee wrote: > HI Ondřej, > > On Mon, May 13, 2019 at 6:16 AM Ondřej Jirman wrote: > > > + > > > +/* Temp Unit: millidegree Celsius */ > > > +static int tsens_reg2temp(struct tsens_device *tmdev, > > > + int reg) > > > > Please name all functions so that they are more clearly identifiable > > in stack traces as belonging to this driver. For example: > > > > sun8i_ths_reg2temp > > > > The same applies for all tsens_* functions below. tsens_* is too > > generic. > > Done but no sun8i_ths_reg2temp. > > ths_reg2tem() should be a generic func. > I think it should be suitable for all platforms, so no platform prefix. You've missed my point. The driver name is sun8i_thermal and if you get and oops from the kernel you'll get a stack trace where there are just function names. If you use too generic function names, it will not be clear which driver is oopsing. - sun8i_ths_reg2temp will tell you much more clearly where to search than - ths_reg2temp Of course you can always grep, but most thermal drivers are thermal sensor (ths) drivers, and if multiple of them used this too-generic naming scheme you'd have hard time debugging. Look at other thermal drivers. They usually encode driver name in the function names to help with identification (even if these are static driver-local functions). > > > +static int tsens_probe(struct platform_device *pdev) > > > +{ > > > + struct tsens_device *tmdev; > > > + struct device *dev = &pdev->dev; > > > + int ret; > > > + > > > + tmdev = devm_kzalloc(dev, sizeof(*tmdev), GFP_KERNEL); > > > + if (!tmdev) > > > + return -ENOMEM; > > > + > > > + tmdev->dev = dev; > > > + tmdev->chip = of_device_get_match_data(&pdev->dev); > > > + if (!tmdev->chip) > > > + return -EINVAL; > > > + > > > + ret = tsens_init(tmdev); > > > + if (ret) > > > + return ret; > > > + > > > + ret = tsens_register(tmdev); > > > + if (ret) > > > + return ret; > > > > Why split this out of probe into separate functions? > > > > > + ret = tmdev->chip->enable(tmdev); > > > + if (ret) > > > + return ret; > > > + > > > + platform_set_drvdata(pdev, tmdev); > > > + > > > + return ret; > > > +} > > > + > > > +static int tsens_remove(struct platform_device *pdev) > > > +{ > > > + struct tsens_device *tmdev = platform_get_drvdata(pdev); > > > + > > > + tmdev->chip->disable(tmdev); > > > + > > > + return 0; > > > +} > > > + > > > +static int sun50i_thermal_enable(struct tsens_device *tmdev) > > > +{ > > > + int ret, val; > > > + > > > + ret = reset_control_deassert(tmdev->reset); > > > + if (ret) > > > + return ret; > > > + > > > + ret = clk_prepare_enable(tmdev->bus_clk); > > > + if (ret) > > > + goto assert_reset; > > > + > > > + ret = tsens_calibrate(tmdev); > > > + if (ret) > > > + return ret; > > > > If this fails (it may likely fail with EPROBE_DEFER) you are leaving reset > > deasserted, and clock enabled. > > > > Overall, I think, reset/clock management and nvmem reading will be common > > to all the HW variants, so it doesn't make much sense splitting it out > > of probe into separate functions, and makes it more error prone. > > Our long-term goal is to support all platforms. > Bacicallt there is a differencr between each generation. > So I feel it necessary to isolate these differences. > > Maybe: > At some point, we can draw a part of the public part and platform > difference into different > files. something like qcom thermal driver. I understand, but I wrote ths drivers for H3/H5/A83T and it so far it looks like all of them would share these 3 calls. You'll be enabling clock/reset and callibrating everywhere. So putting this to per-SoC function seems premature. thank you and regards, o. > Regards, > Yangtao