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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 3F0E2FA3742 for ; Fri, 28 Oct 2022 15:36:31 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=FtdDwh5iDtzgIWCwVuB5jL6EaiKxboghGoFlvl+QY8s=; b=r2t/euQPYanlV/ 4oURCVEZTIMt7Dj7i+Ux/iAdnX4hYSNtUnnfBUxBPHd1lGmJNt5BYopETVe9zTXD9/KtPgNpK41sH ThUyZSqYd/jO+nqodG7YjUoj1yOisFS0QoDYw11N4Nuzi8aOfQ0aKv0AWEQpaIEi+zVyIGhzYyBAV Xsr+RWgOZzsfilei8BEoaeWdAPPoRl4jaxKgIemOPbjbNwbEB4fk99S5Y/py3QuQKanf//+JBmrEn 29cMwdzfEpj1JtZjqxToRhJGgE2Qdbgqptt9aaB4iCH7duBAMJYWfimLSgQSk5PIV/mXl4zbQOR3T BlsWD6/bf9VcBlIwIqZg==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1ooROJ-000Y4y-Cl; Fri, 28 Oct 2022 15:35:27 +0000 Received: from foss.arm.com ([217.140.110.172]) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1ooROE-000Y3I-KB for linux-arm-kernel@lists.infradead.org; Fri, 28 Oct 2022 15:35:26 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 112181FB; Fri, 28 Oct 2022 08:35:26 -0700 (PDT) Received: from e120937-lin (unknown [172.31.20.19]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id D28323F534; Fri, 28 Oct 2022 08:35:18 -0700 (PDT) Date: Fri, 28 Oct 2022 16:35:12 +0100 From: Cristian Marussi To: Guenter Roeck Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, sudeep.holla@arm.com, Daniel Lezcano , linux-hwmon@vger.kernel.org Subject: Re: [PATCH 7/8] hwmon: (scmi) Register explicitly with Thermal Framework Message-ID: References: <20221028140833.280091-1-cristian.marussi@arm.com> <20221028140833.280091-7-cristian.marussi@arm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20221028_083522_784261_B2282947 X-CRM114-Status: GOOD ( 38.13 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Fri, Oct 28, 2022 at 08:11:59AM -0700, Guenter Roeck wrote: > On 10/28/22 07:08, Cristian Marussi wrote: > > Available sensors are enumerated and reported by the SCMI platform server > > using a 16bit identification number; not all such sensors are of a type > > supported by hwmon subsystem and, among the supported ones, only a subset > > could be temperature sensors that have to be registered with the Thermal > > Framework. > > Potential clashes between hwmon channels indexes and the underlying real > > sensors IDs do not play well with the hwmon<-->thermal bridge automatic > > registration routines and could need a sensible number of fake dummy > > sensors to be made up in order to keep indexes and IDs in sync. > > > > Avoid to use the hwmon<-->thermal bridge dropping the HWMON_C_REGISTER_TZ > > attribute and instead explicit register temperature sensors directly with > > the Thermal Framework. > > > > > For my reference: > > Reviewed-by: Guenter Roeck > > $subject says "patch 7/8". Patches 1-6 are firmware patches. Does this patch > depend on the other patches of the series or can I apply it on its own ? Thanks for having a look first of all ! This patch can be applied on its own...it's just that I have bundled together a bunch of fixes (... this being probably a bit too big really it should have been on its own, sorry for that...) > > Additional comment inline below. > > Thanks, > Guenter > > > Cc: Daniel Lezcano > > Cc: Guenter Roeck > > Cc: linux-hwmon@vger.kernel.org > > Signed-off-by: Cristian Marussi > > --- > > drivers/hwmon/scmi-hwmon.c | 115 ++++++++++++++++++++++++++++++++----- > > 1 file changed, 102 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c > > index b1329a58ce40..124fe8ee1b9b 100644 > > --- a/drivers/hwmon/scmi-hwmon.c > > +++ b/drivers/hwmon/scmi-hwmon.c > > @@ -20,6 +20,11 @@ struct scmi_sensors { > > const struct scmi_sensor_info **info[hwmon_max]; > > }; > > +struct scmi_thermal_sensor { > > + const struct scmi_protocol_handle *ph; > > + const struct scmi_sensor_info *info; > > +}; > > + > > static inline u64 __pow10(u8 x) > > { > > u64 r = 1; > > @@ -64,16 +69,14 @@ static int scmi_hwmon_scale(const struct scmi_sensor_info *sensor, u64 *value) > > return 0; > > } > > -static int scmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type, > > - u32 attr, int channel, long *val) > > +static int scmi_hwmon_read_scaled_value(const struct scmi_protocol_handle *ph, > > + const struct scmi_sensor_info *sensor, > > + long *val) > > { > > int ret; > > u64 value; > > - const struct scmi_sensor_info *sensor; > > - struct scmi_sensors *scmi_sensors = dev_get_drvdata(dev); > > - sensor = *(scmi_sensors->info[type] + channel); > > - ret = sensor_ops->reading_get(scmi_sensors->ph, sensor->id, &value); > > + ret = sensor_ops->reading_get(ph, sensor->id, &value); > > if (ret) > > return ret; > > @@ -84,6 +87,17 @@ static int scmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type, > > return ret; > > } > > +static int scmi_hwmon_read(struct device *dev, enum hwmon_sensor_types type, > > + u32 attr, int channel, long *val) > > +{ > > + const struct scmi_sensor_info *sensor; > > + struct scmi_sensors *scmi_sensors = dev_get_drvdata(dev); > > + > > + sensor = *(scmi_sensors->info[type] + channel); > > + > > + return scmi_hwmon_read_scaled_value(scmi_sensors->ph, sensor, val); > > +} > > + > > static int > > scmi_hwmon_read_string(struct device *dev, enum hwmon_sensor_types type, > > u32 attr, int channel, const char **str) > > @@ -122,6 +136,25 @@ static struct hwmon_chip_info scmi_chip_info = { > > .info = NULL, > > }; > > +static int scmi_hwmon_thermal_get_temp(struct thermal_zone_device *tz, > > + int *temp) > > +{ > > + int ret; > > + long value; > > + struct scmi_thermal_sensor *th_sensor = tz->devdata; > > + > > + ret = scmi_hwmon_read_scaled_value(th_sensor->ph, th_sensor->info, > > + &value); > > + if (!ret) > > + *temp = value; > > + > > + return ret; > > +} > > + > > +static const struct thermal_zone_device_ops scmi_hwmon_thermal_ops = { > > + .get_temp = scmi_hwmon_thermal_get_temp, > > +}; > > + > > static int scmi_hwmon_add_chan_info(struct hwmon_channel_info *scmi_hwmon_chan, > > struct device *dev, int num, > > enum hwmon_sensor_types type, u32 config) > > @@ -149,7 +182,6 @@ static enum hwmon_sensor_types scmi_types[] = { > > }; > > static u32 hwmon_attributes[hwmon_max] = { > > - [hwmon_chip] = HWMON_C_REGISTER_TZ, > > [hwmon_temp] = HWMON_T_INPUT | HWMON_T_LABEL, > > [hwmon_in] = HWMON_I_INPUT | HWMON_I_LABEL, > > [hwmon_curr] = HWMON_C_INPUT | HWMON_C_LABEL, > > @@ -157,6 +189,43 @@ static u32 hwmon_attributes[hwmon_max] = { > > [hwmon_energy] = HWMON_E_INPUT | HWMON_E_LABEL, > > }; > > +static int scmi_thermal_sensor_register(struct device *dev, > > + const struct scmi_protocol_handle *ph, > > + const struct scmi_sensor_info *sensor) > > +{ > > + struct scmi_thermal_sensor *th_sensor; > > + struct thermal_zone_device *tzd; > > + > > + th_sensor = devm_kzalloc(dev, sizeof(*th_sensor), GFP_KERNEL); > > + if (!th_sensor) > > + return -ENOMEM; > > + > > + th_sensor->ph = ph; > > + th_sensor->info = sensor; > > + > > + /* > > + * Try to register a temperature sensor with the Thermal Framework: > > + * skip sensors not defined as part of any thermal zone (-ENODEV) but > > + * report any other errors related to misconfigured zones/sensors. > > + */ > > + tzd = devm_thermal_of_zone_register(dev, th_sensor->info->id, th_sensor, > > + &scmi_hwmon_thermal_ops); > > + if (IS_ERR(tzd)) { > > + devm_kfree(dev, th_sensor); > > + > > + if (PTR_ERR(tzd) != -ENODEV) > > + return PTR_ERR(tzd); > > + > > + dev_info(dev, "Sensor '%s' not attached to any thermal zone.\n", > > + sensor->name); > > There were complaints about this message as it is noisy. If you send > another version, please drop it unless attaching each sensor to a thermal > zone is strongly expected. If you don't send another version, I'll drop it > while applying. > Ok fine for me. I am waiting to have some feedback from Sudeep too, but I do not have plan for another version as of now. As a side note, though, I understand the 'noisiness' argument, but, sincerely this same message in the original HWMON code was the only reason why I spotted that something was wrong with the SCMI/HWMON interactions and discovered the indexes/ids mismatch...if not for that it would have gone un-noticed that a perfectly configured ThermalZone/Sensor was not working properly... (un-noticed at least until something would have been burnt to fire in my house .. joking :P) Thanks, Cristian _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel