Received-SPF: pass (google.com: domain of linux-kernel+bounces-32800-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Mon, 22 Jan 2024 10:59:14 +0000
From: Cristian Marussi <cristian.marussi@arm.com>
To: "Peng Fan (OSS)" <peng.fan@oss.nxp.com>
Cc: sudeep.holla@arm.com, jdelvare@suse.com, linux@roeck-us.net,
	linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org,
	Peng Fan <peng.fan@nxp.com>
Subject: Re: [PATCH] hwmon: scmi-hwmon: implement change_mode
Message-ID: <Za5Kgp6_qUEMc-AO@pluto>
References: <20240122080441.1957022-1-peng.fan@oss.nxp.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240122080441.1957022-1-peng.fan@oss.nxp.com>

On Mon, Jan 22, 2024 at 04:04:41PM +0800, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
> 
> The sensor maybe disabled before kernel boot, so add change_mode
> to support configuring the sensor to enabled state.
> 

Hi Peng,

minor remarks down below

> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
>  drivers/hwmon/scmi-hwmon.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/hwmon/scmi-hwmon.c b/drivers/hwmon/scmi-hwmon.c
> index 364199b332c0..f7bd63d52d15 100644
> --- a/drivers/hwmon/scmi-hwmon.c
> +++ b/drivers/hwmon/scmi-hwmon.c
> @@ -151,7 +151,36 @@ static int scmi_hwmon_thermal_get_temp(struct thermal_zone_device *tz,
>  	return ret;
>  }
>  
> +static int scmi_hwmon_thermal_change_mode(struct thermal_zone_device *tz,
> +					  enum thermal_device_mode new_mode)
> +{
> +	int ret;
> +	u32 config;
> +	enum thermal_device_mode cur_mode = THERMAL_DEVICE_DISABLED;
> +	struct scmi_thermal_sensor *th_sensor = thermal_zone_device_priv(tz);
> +
> +	ret = sensor_ops->config_get(th_sensor->ph, th_sensor->info->id,
> +				     &config);
> +	if (ret)
> +		return ret;
> +
> +	if (config & BIT(0))

if SCMI_SENS_CFG_IS_ENABLED(config)

> +		cur_mode = THERMAL_DEVICE_ENABLED;
> +
> +	if (cur_mode == new_mode)
> +		return 0;
> +
> +	if (new_mode == THERMAL_DEVICE_ENABLED)
> +		config |= SCMI_SENS_CFG_SENSOR_ENABLED_MASK;
> +	else
> +		config &= ~SCMI_SENS_CFG_SENSOR_ENABLED_MASK;
> +

Here you are ORing the enable bit to preserve the value obtained by
config_get (rightly so), BUT unfortunately looking at the spec
CONFIG_SET uses bits [10:9] for setting the rounding mode while
CONFIG_GET does NOT report (bits [10:2] are Reserved) so you are
in fact setting ROUNDING to 00 probably, meaning ROUND_DOWN.

We could have to add (in the future not now) something in CONFIG_GET
to get the round mode (I'll speak with Souvik) BUT in the meantime I
wonder if it is not more safe here to just explicitly set the mode to
10 to signify it is up to the platform to autonomously choose how to
round. (there are SCMI_SENS_CFG_ROUND_ macros and mask in
scmi_protocol.h)

Other than these small things, LGTM.

Reviewed-by: Cristian Marussi <cristian.marussi@arm.com>

Thanks,
Cristian