Received: by 2002:a25:86ce:0:0:0:0:0 with SMTP id y14csp779713ybm; Tue, 21 May 2019 03:28:48 -0700 (PDT) X-Google-Smtp-Source: APXvYqwMDnGpFUCgmyu0X2f4/1jpuZIg7tXQlFpU9n6ZqHwsszRlHAMnFi/sXO9jZnrqh7FlL3jv X-Received: by 2002:a62:585:: with SMTP id 127mr66142004pff.231.1558434528603; Tue, 21 May 2019 03:28:48 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1558434528; cv=none; d=google.com; s=arc-20160816; b=I+XEdIoWaDBduFI50DlS37H8Unj9fDdCo4hnqbijWGpjw8pSIxdx04aP+/7dDwyyvz fk007ngYv5TzEsJns0UsvD85xDsxpYqgfMH1L5LfmH47cq1J+0bMACEzAQx+/Z3BpjqK XafmxMFlMhbO2OmmaqmZ+mfzmXp/DjKk5w8cWzQPh8Qv5tv5ZUi3uMts+IxkiQ20eA8g yBILFRZCLcLh+kovKr+VpMAghxraW7ISdDpSwnITEiTx8nEdexuhHaSEAY0FVduCziOt dJ69KA/NwxXKzlkvXbft4YJSRKmG6V1LkVGol6PMjZJL1GjtT/0bz6Qr9JFiyZ8CRUCz azUw== 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=Hb+r/zZZAHWmTIigWTeIyAqBbgLula1ItgVRzKfPdLE=; b=aidTWDqIje06LCKw3g0Y4ytj/U74+WAuoQVVWvll/c88cZXNS8i5+YGP1x5SdmCK44 pIxLTSGgAGEO376oYI1C95QJKwnkuuyGjWD8EQSzshY2+ywvuLbTr2XzSERiRBhM6PyD mCuH2o9uA4xS39OBlVJSCTQUbeCbhrZ3JS2n2j2b5Loh9xaj9/e/7Ha871eSRc9Bw/HX gZzKLkrtgKAYo00Qp1s+skc2wzJkMomyu/9heQXVIxeATnAEQc2Bul/Lf2KRrpZfGs/U uhBhopMDlkO/FtghcaNHMaqAjGBOLe2mn47Ts3YhArog+CSReLqDgPtEvpSSuTbbqGl4 Ll3w== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@megous.com header.s=mail header.b=m29HCqX2; 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 14si10099105pfw.143.2019.05.21.03.28.33; Tue, 21 May 2019 03:28:48 -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=m29HCqX2; 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 S1727530AbfEUK1Z (ORCPT + 99 others); Tue, 21 May 2019 06:27:25 -0400 Received: from vps.xff.cz ([195.181.215.36]:38106 "EHLO vps.xff.cz" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726344AbfEUK1Z (ORCPT ); Tue, 21 May 2019 06:27:25 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=megous.com; s=mail; t=1558434441; bh=H6MrKqqtdax1I2XXyrIgpDdBNiCoz5e/dUQ3W4MknQk=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=m29HCqX2SN+8L1d5/7f5EvQHqPXlEn9XXs31rwizmVmEdBITHL/a3TU6OMvTBHDcw yUjXEsxaziEiajsSIS0H84WyhAC2+H0eBJWcb+jZqN2avYXfv90LK3Q1e+1qfkVgAJ uJODrBg6bk4nHXnynQwSIMwDEaWMaD5TL+M7adhA= Date: Tue, 21 May 2019 12:27:21 +0200 From: =?utf-8?Q?Ond=C5=99ej?= Jirman To: Maxime Ripard Cc: Frank Lee , Mark Rutland , Daniel Lezcano , catalin.marinas@arm.com, will.deacon@arm.com, bjorn.andersson@linaro.org, Mauro Carvalho Chehab , paulmck@linux.ibm.com, stefan.wahren@i2se.com, Linux PM , Chen-Yu Tsai , Jagan Teki , Andy Gross , rui.zhang@intel.com, devicetree@vger.kernel.org, marc.w.gonzalez@free.fr, Eduardo Valentin , enric.balletbo@collabora.com, robh+dt@kernel.org, Jonathan.Cameron@huawei.com, Linux ARM , Greg Kroah-Hartman , Linux Kernel Mailing List , olof@lixom.net, David Miller Subject: Re: [PATCH 2/3] thermal: sun50i: add thermal driver for h6 Message-ID: <20190521102721.5hgks6guzlhubj6d@core.my.home> Mail-Followup-To: Maxime Ripard , Frank Lee , Mark Rutland , Daniel Lezcano , catalin.marinas@arm.com, will.deacon@arm.com, bjorn.andersson@linaro.org, Mauro Carvalho Chehab , paulmck@linux.ibm.com, stefan.wahren@i2se.com, Linux PM , Chen-Yu Tsai , Jagan Teki , Andy Gross , rui.zhang@intel.com, devicetree@vger.kernel.org, marc.w.gonzalez@free.fr, Eduardo Valentin , enric.balletbo@collabora.com, robh+dt@kernel.org, Jonathan.Cameron@huawei.com, Linux ARM , Greg Kroah-Hartman , Linux Kernel Mailing List , olof@lixom.net, David Miller References: <20190512082614.9045-1-tiny.windzz@gmail.com> <20190512082614.9045-3-tiny.windzz@gmail.com> <20190512133930.t5txssl7mou2gljt@flea> <20190517073634.izdmba3yqvxviyg3@flea> <20190521080515.qlni2lnmcwh7itl7@flea> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20190521080515.qlni2lnmcwh7itl7@flea> Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Maxime, On Tue, May 21, 2019 at 10:05:15AM +0200, Maxime Ripard wrote: > On Sat, May 18, 2019 at 01:27:39AM +0800, Frank Lee wrote: > > On Fri, May 17, 2019 at 3:36 PM Maxime Ripard wrote: > > > > > > On Fri, May 17, 2019 at 01:51:56AM +0800, Frank Lee wrote: > > > > > > +struct sun50i_thermal_chip { > > > > > > + int sensor_num; > > > > > > + int offset; > > > > > > + int scale; > > > > > > + int ft_deviation; > > > > > > + int temp_calib_base; > > > > > > + int temp_data_base; > > > > > > + int (*enable)(struct tsens_device *tmdev); > > > > > > + int (*disable)(struct tsens_device *tmdev); > > > > > > +}; > > > > > > > > > > I'm not super fond of having a lot of quirks that are not needed. If > > > > > we ever need those quirks when adding support for a new SoC, then > > > > > yeah, we should totally have some, but only when and if it's needed. > > > > > > > > > > Otherwise, the driver is more complicated for no particular reason. > > > > > > > > This is unavoidable because of the difference in soc. > > > > > > I know, but this isn't my point. > > > > > > My point is that at this time of the driver development, we don't know > > > what is going to be needed to support all of those SoCs. > > > > > > Some of the parameters you added might not be needed, some parameters > > > might be missing, we don't know. So let's keep it simple for now. > > > > > > > > > +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; > > > > > > + > > > > > > + ret = tmdev->chip->enable(tmdev); > > > > > > + if (ret) > > > > > > + return ret; > > > > > > > > > > > > + platform_set_drvdata(pdev, tmdev); > > > > > > > > > > Your registration should be the very last thing you do. Otherwise, you > > > > > have a small window where the get_temp callback can be called, but the > > > > > driver will not be functional yet. > > > > > > > > No. Anyway, ths data qcquisition is ms level. > > > > > > That's kind of irrelevant. There's nothing preventing get_temp to be > > > called right away. > > > > As Ondřej said, > > > > Registration after enabling will lead to call tz update on non-registered tz > > from an interrupt handler. > > I'm probably missing something but you're not using the interrupts, so > how could an interrupt handler call it? > > Also, other drivers seem to be doing that just fine (mtk_thermal for > example), so surely there's a way? Last version is using the interrupts. Drivers do it in various ways. For example imx_thermal (and others like hisi_thermal) does it the suggested way. It enables interrupts after thermal zone registration, so that IRQ handler doesn't get invoked before the tzd is registered. regards, o. > > > > > > + ret = tsens_calibrate(tmdev); > > > > > > + if (ret) > > > > > > + return ret; > > > > > > + > > > > > > + /* > > > > > > + * clkin = 24MHz > > > > > > + * T acquire = clkin / (SUN50I_THS_CTRL0_T_ACQ + 1) > > > > > > + * = 20us > > > > > > + */ > > > > > > + regmap_write(tmdev->regmap, SUN50I_THS_CTRL0, > > > > > > + SUN50I_THS_CTRL0_T_ACQ(479)); > > > > > > + /* average over 4 samples */ > > > > > > + regmap_write(tmdev->regmap, SUN50I_H6_THS_MFC, > > > > > > + SUN50I_THS_FILTER_EN | > > > > > > + SUN50I_THS_FILTER_TYPE(1)); > > > > > > + /* period = (SUN50I_H6_THS_PC_TEMP_PERIOD + 1) * 4096 / clkin; ~10ms */ > > > > > > + regmap_write(tmdev->regmap, SUN50I_H6_THS_PC, > > > > > > + SUN50I_H6_THS_PC_TEMP_PERIOD(58)); > > > > > > + /* enable sensor */ > > > > > > + val = GENMASK(tmdev->chip->sensor_num - 1, 0); > > > > > > + regmap_write(tmdev->regmap, SUN50I_H6_THS_ENABLE, val); > > > > > > + > > > > > > + return 0; > > > > > > + > > > > > > +assert_reset: > > > > > > + reset_control_assert(tmdev->reset); > > > > > > + > > > > > > + return ret; > > > > > > > > > > Can't we do that with runtime_pm? > > > > > > > > Saving energy doesn't make much sense compared to system security. > > > > > > I'm not sure what you mean by security. > > > > Protect system hardware from damage. > > The point of runtime_pm is to keep the device on as long as it is > used, so it wouldn't change anything there. > > I mean, you can even enable it in the probe if you want, my point is > that the hooks that you have are exact equivalents to the one provided > by runtime_pm already, so there's no need to define them in the first > place. > > Maxime > > -- > Maxime Ripard, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel