Received-SPF: pass (google.com: domain of linux-kernel+bounces-33157-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Date: Mon, 22 Jan 2024 14:28:26 +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: <Za57itGEQtLqCNcg@pluto>
References: <20240122080441.1957022-1-peng.fan@oss.nxp.com>
 <Za5Kgp6_qUEMc-AO@pluto>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Za5Kgp6_qUEMc-AO@pluto>

On Mon, Jan 22, 2024 at 10:59:18AM +0000, Cristian Marussi wrote:
> 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)

..so after a quick check with ATG, this ROUNDING mode is indeed related
to this specific CONFIG_SET request being issued...it is not a general
sensor config, so it is not and will not be needed to be reported from
CONFIG_GET (and was indeed clear from the spec my bad...).

Moreover looking better at SENSOR_CONFIG_SET spec the sensor_update_interval
field can be set to 0 if not required to be updated....so I suppose the
better policy here is to just clear out all the bits [31:9] before setting
the enable bit, while KEEPING the original timestamp bit untouched.

A bit tricky...sorry for the confusion.

Thanks,
Cristian