drivers/hwmon/cros_ec_hwmon.c | 41 +++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+)
From: "Sung-Chi, Li" <lschyi@chromium.org>
cros_ec hwmon driver probes available thermal sensors when probing the
driver. Register these thermal sensors to the thermal framework, such
that thermal framework can adopt these sensors as well.
To make cros_ec registrable to thermal framework, the cros_ec dts need
the corresponding changes:
&cros_ec {
#thermal-sensor-cells = <1>;
};
Change-Id: I29b638427c715cb44391496881fc61ad53abccaf
Signed-off-by: Sung-Chi, Li <lschyi@chromium.org>
---
Changes in v2:
- Rename `cros_ec_sensor_data` to `cros_ec_hwmon_thermal_zone_data`.
- Rename `addr` in struct `cros_ec_hwmon_thermal_zone_data` to `idx`.
- Use `cros_ec_hwmon_temp_to_millicelsius` to do value conversion in
`cros_ec_thermal_get_temp` function.
- Rename `cros_ec_thermal_get_temp` to `cros_ec_hwmon_thermal_get_temp` to
make `cros_ec_hwmon` a prefix.
- Use `%pe` in `cros_ec_hwmon_probe_temp_sensors` when printing out
`data->tz_dev` if failed register thermal device.
- Remove `cros_ec_hwmon_remove`, and the `.remove` value in
`cros_ec_hwmon_driver` since there is no need to call
`devm_thermal_of_zone_unregister` for clean up.
- Revert function signature of `cros_ec_hwmon_probe_temp_sensors` since all
needed parameters are presented.
- Revert include of `linux/list.h` because no list data structure is used.
---
drivers/hwmon/cros_ec_hwmon.c | 41 +++++++++++++++++++++++++++++++++++
1 file changed, 41 insertions(+)
diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
index 5514cf780b8b..81e563e0455f 100644
--- a/drivers/hwmon/cros_ec_hwmon.c
+++ b/drivers/hwmon/cros_ec_hwmon.c
@@ -12,6 +12,7 @@
#include <linux/platform_device.h>
#include <linux/platform_data/cros_ec_commands.h>
#include <linux/platform_data/cros_ec_proto.h>
+#include <linux/thermal.h>
#include <linux/types.h>
#include <linux/units.h>
@@ -23,6 +24,12 @@ struct cros_ec_hwmon_priv {
u8 usable_fans;
};
+struct cros_ec_hwmon_thermal_zone_data {
+ struct cros_ec_device *cros_ec;
+ struct thermal_zone_device *tz_dev;
+ int idx;
+};
+
static int cros_ec_hwmon_read_fan_speed(struct cros_ec_device *cros_ec, u8 index, u16 *speed)
{
int ret;
@@ -185,11 +192,30 @@ static const struct hwmon_chip_info cros_ec_hwmon_chip_info = {
.info = cros_ec_hwmon_info,
};
+static int cros_ec_hwmon_thermal_get_temp(struct thermal_zone_device *tz, int *temp)
+{
+ struct cros_ec_hwmon_thermal_zone_data *data =
+ thermal_zone_device_priv(tz);
+ int ret;
+ u8 val;
+
+ ret = cros_ec_hwmon_read_temp(data->cros_ec, data->idx, &val);
+ if (ret || cros_ec_hwmon_is_error_temp(temp))
+ return -ENODATA;
+ *temp = cros_ec_hwmon_temp_to_millicelsius(val);
+ return 0;
+}
+
+static const struct thermal_zone_device_ops thermal_ops = {
+ .get_temp = cros_ec_hwmon_thermal_get_temp,
+};
+
static void cros_ec_hwmon_probe_temp_sensors(struct device *dev, struct cros_ec_hwmon_priv *priv,
u8 thermal_version)
{
struct ec_params_temp_sensor_get_info req = {};
struct ec_response_temp_sensor_get_info resp;
+ struct cros_ec_hwmon_thermal_zone_data *data;
size_t candidates, i, sensor_name_size;
int ret;
u8 temp;
@@ -216,6 +242,21 @@ static void cros_ec_hwmon_probe_temp_sensors(struct device *dev, struct cros_ec_
priv->temp_sensor_names[i] = devm_kasprintf(dev, GFP_KERNEL, "%.*s",
(int)sensor_name_size,
resp.sensor_name);
+
+ data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
+ if (!data)
+ continue;
+
+ data->idx = i;
+ data->cros_ec = priv->cros_ec;
+ data->tz_dev = devm_thermal_of_zone_register(
+ priv->cros_ec->dev, i, data, &thermal_ops);
+ if (IS_ERR_VALUE(data->tz_dev)) {
+ dev_err(dev,
+ "failed to register %zu thermal sensor, err = %pe",
+ i, data->tz_dev);
+ continue;
+ }
}
}
--
2.47.0.277.g8800431eea-googOn 11/11/24 01:50, Sung-Chi wrote:
> From: "Sung-Chi, Li" <lschyi@chromium.org>
>
> cros_ec hwmon driver probes available thermal sensors when probing the
> driver. Register these thermal sensors to the thermal framework, such
> that thermal framework can adopt these sensors as well.
>
> To make cros_ec registrable to thermal framework, the cros_ec dts need
> the corresponding changes:
>
> &cros_ec {
> #thermal-sensor-cells = <1>;
> };
>
> Change-Id: I29b638427c715cb44391496881fc61ad53abccaf
Drop.
> Signed-off-by: Sung-Chi, Li <lschyi@chromium.org>
Detailed explanation will be needed: Why not use HWMON_C_REGISTER_TZ ?
Unless I am missing something, this code just duplicates code from the hwmon core.
Please do not send follow-up patch series as response to previous ones.
Guenter
On Mon, Nov 11, 2024 at 09:01:33AM -0800, Guenter Roeck wrote:
> On 11/11/24 01:50, Sung-Chi wrote:
> > From: "Sung-Chi, Li" <lschyi@chromium.org>
> >
> > cros_ec hwmon driver probes available thermal sensors when probing the
> > driver. Register these thermal sensors to the thermal framework, such
> > that thermal framework can adopt these sensors as well.
> >
> > To make cros_ec registrable to thermal framework, the cros_ec dts need
> > the corresponding changes:
> >
> > &cros_ec {
> > #thermal-sensor-cells = <1>;
> > };
> >
> > Change-Id: I29b638427c715cb44391496881fc61ad53abccaf
>
> Drop.
>
> > Signed-off-by: Sung-Chi, Li <lschyi@chromium.org>
>
> Detailed explanation will be needed: Why not use HWMON_C_REGISTER_TZ ?
> Unless I am missing something, this code just duplicates code from the hwmon core.
>
> Please do not send follow-up patch series as response to previous ones.
>
> Guenter
>
Hi, thank you for pointing out using HWMON_C_REGISTER_TZ. After checking how
HWMON_C_REGSITER_TZ works, I think I only need to add one line into the
cros_ec_hwmon_info, and almost all concerns Thomas pointed out in latest reply
would be resolved automatically (because there would be only one line of change,
and that change is just a hwmon configuration, so should be a valid way of
combining with the thermal system).
Thank all for reviewing and giving inputs, and I will soon send out the one
line patch.
Best,
Sung-Chi, Li
On 2024-11-11 17:50:30+0800, Sung-Chi wrote:
> From: "Sung-Chi, Li" <lschyi@chromium.org>
>
> cros_ec hwmon driver probes available thermal sensors when probing the
> driver. Register these thermal sensors to the thermal framework, such
> that thermal framework can adopt these sensors as well.
The driver also supports fan readings. These could also be wired up as
cooling devices.
> To make cros_ec registrable to thermal framework, the cros_ec dts need
> the corresponding changes:
>
> &cros_ec {
> #thermal-sensor-cells = <1>;
> };
If this is the only thing that is meant to be configured I'm wondering
why the OF variant is needed in the first place.
Why not register a non-OF thermal device?
Please send the next revision also to the maintainers of the THERMAL
subsystem so we can figure out the most correct way forward.
> Change-Id: I29b638427c715cb44391496881fc61ad53abccaf
> Signed-off-by: Sung-Chi, Li <lschyi@chromium.org>
> ---
> Changes in v2:
> - Rename `cros_ec_sensor_data` to `cros_ec_hwmon_thermal_zone_data`.
> - Rename `addr` in struct `cros_ec_hwmon_thermal_zone_data` to `idx`.
> - Use `cros_ec_hwmon_temp_to_millicelsius` to do value conversion in
> `cros_ec_thermal_get_temp` function.
> - Rename `cros_ec_thermal_get_temp` to `cros_ec_hwmon_thermal_get_temp` to
> make `cros_ec_hwmon` a prefix.
> - Use `%pe` in `cros_ec_hwmon_probe_temp_sensors` when printing out
> `data->tz_dev` if failed register thermal device.
> - Remove `cros_ec_hwmon_remove`, and the `.remove` value in
> `cros_ec_hwmon_driver` since there is no need to call
> `devm_thermal_of_zone_unregister` for clean up.
> - Revert function signature of `cros_ec_hwmon_probe_temp_sensors` since all
> needed parameters are presented.
> - Revert include of `linux/list.h` because no list data structure is used.
> ---
> drivers/hwmon/cros_ec_hwmon.c | 41 +++++++++++++++++++++++++++++++++++
> 1 file changed, 41 insertions(+)
>
> diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c
> index 5514cf780b8b..81e563e0455f 100644
> --- a/drivers/hwmon/cros_ec_hwmon.c
> +++ b/drivers/hwmon/cros_ec_hwmon.c
> @@ -12,6 +12,7 @@
> #include <linux/platform_device.h>
> #include <linux/platform_data/cros_ec_commands.h>
> #include <linux/platform_data/cros_ec_proto.h>
> +#include <linux/thermal.h>
> #include <linux/types.h>
> #include <linux/units.h>
>
> @@ -23,6 +24,12 @@ struct cros_ec_hwmon_priv {
> u8 usable_fans;
> };
>
> +struct cros_ec_hwmon_thermal_zone_data {
> + struct cros_ec_device *cros_ec;
> + struct thermal_zone_device *tz_dev;
> + int idx;
> +};
> +
> static int cros_ec_hwmon_read_fan_speed(struct cros_ec_device *cros_ec, u8 index, u16 *speed)
> {
> int ret;
> @@ -185,11 +192,30 @@ static const struct hwmon_chip_info cros_ec_hwmon_chip_info = {
> .info = cros_ec_hwmon_info,
> };
>
> +static int cros_ec_hwmon_thermal_get_temp(struct thermal_zone_device *tz, int *temp)
> +{
> + struct cros_ec_hwmon_thermal_zone_data *data =
> + thermal_zone_device_priv(tz);
> + int ret;
> + u8 val;
> +
> + ret = cros_ec_hwmon_read_temp(data->cros_ec, data->idx, &val);
> + if (ret || cros_ec_hwmon_is_error_temp(temp))
> + return -ENODATA;
> + *temp = cros_ec_hwmon_temp_to_millicelsius(val);
> + return 0;
> +}
> +
> +static const struct thermal_zone_device_ops thermal_ops = {
Symbol still needs namespacing.
> + .get_temp = cros_ec_hwmon_thermal_get_temp,
> +};
> +
> static void cros_ec_hwmon_probe_temp_sensors(struct device *dev, struct cros_ec_hwmon_priv *priv,
> u8 thermal_version)
> {
> struct ec_params_temp_sensor_get_info req = {};
> struct ec_response_temp_sensor_get_info resp;
> + struct cros_ec_hwmon_thermal_zone_data *data;
> size_t candidates, i, sensor_name_size;
> int ret;
> u8 temp;
> @@ -216,6 +242,21 @@ static void cros_ec_hwmon_probe_temp_sensors(struct device *dev, struct cros_ec_
> priv->temp_sensor_names[i] = devm_kasprintf(dev, GFP_KERNEL, "%.*s",
> (int)sensor_name_size,
> resp.sensor_name);
> +
> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> + if (!data)
> + continue;
> +
> + data->idx = i;
> + data->cros_ec = priv->cros_ec;
> + data->tz_dev = devm_thermal_of_zone_register(
> + priv->cros_ec->dev, i, data, &thermal_ops);
Doesn't this also automatically create new hwmon device off of the
thermal device? That shouldn't happen.
In general I'm not sure how the hwmon and thermal subsystems are meant
to interact. Is one recommended over the other?
Should the driver become a first-class thermal driver and use the
automatic hwmon functionality?
> + if (IS_ERR_VALUE(data->tz_dev)) {
> + dev_err(dev,
> + "failed to register %zu thermal sensor, err = %pe",
> + i, data->tz_dev);
If !CONFIG_OF || !CONFIG_THERMAL this will always log an error.
EOPNOTSUP should not trigger that logging.
> + continue;
> + }
> }
> }
>
> --
> 2.47.0.277.g8800431eea-goog
>
The cros_ec supports reading thermal values from thermal sensors
connect to it. Add the property '#thermal-sensor-cells' bindings, such
that thermal framework can recognize cros_ec as a valid thermal device.
Change-Id: I95a22c0f1a69de547fede5f0f9c43cbd60820789
Signed-off-by: Sung-Chi <lschyi@chromium.org>
---
Changes in v2:
- Add changes for DTS binding.
---
Documentation/devicetree/bindings/mfd/google,cros-ec.yaml | 3 +++
1 file changed, 3 insertions(+)
diff --git a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
index aac8819bd00b..c7d63e3aacd2 100644
--- a/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
+++ b/Documentation/devicetree/bindings/mfd/google,cros-ec.yaml
@@ -96,6 +96,9 @@ properties:
'#gpio-cells':
const: 2
+ '#thermal-sensor-cells':
+ const: 1
+
gpio-controller: true
typec:
--
2.47.0.277.g8800431eea-googOn Mon, Nov 11, 2024 at 05:50:31PM +0800, Sung-Chi wrote: > The cros_ec supports reading thermal values from thermal sensors > connect to it. Add the property '#thermal-sensor-cells' bindings, such > that thermal framework can recognize cros_ec as a valid thermal device. > > Change-Id: I95a22c0f1a69de547fede5f0f9c43cbd60820789 ^^^^^^^^^ With this removed, Acked-by: Conor Dooley <conor.dooley@microchip.com> Cheers, Conor.
© 2016 - 2024 Red Hat, Inc.