On 2025-03-17 14:40:26+0800, Sung-Chi Li wrote: > The ChromeOS embedded controller (EC) supports closed loop fan speed > control, so add the fan target attribute under hwmon framework, such > that kernel can expose reading and specifying the desired fan RPM for > fans connected to the EC. > > When probing the cros_ec hwmon module, we also check the supported > command version of setting target fan RPM. This commit implements the > version 0 of getting the target fan RPM, which can only read the target > RPM of the first fan. This commit also implements the version 0 and 1 of > setting the target fan RPM, where the version 0 only supports setting > all fan to the same RPM, while version 1 supports setting different RPM > to each fan respectively. Can you explain why this set of command compatibility was chosen? I would have expected to fully support the v1 commands and completely skip v0. > Signed-off-by: Sung-Chi Li <lschyi@xxxxxxxxxxxx> > --- > ChromeOS embedded controller (EC) supports closed-loop fan control. We > anticipate to have the fan related control from the kernel side, so this > series register the HWMON_F_TARGET attribute, and implement the read and > write function for setting/reading the target fan RPM from the EC side. > --- > Changes in v2: > > - Squash the read, write, and register of fan target attribute to 1 > commit, as they are the same topic. > - Probe the supported command version from EC for setting the target fan > RPM, and perform the set fan target RPM based on the supported > version. > - Update the used variable type to kernel types (i.e., u32). > - Link to v1: https://lore.kernel.org/r/20250313-extend_ec_hwmon_fan-v1-0-5c566776f2c4@xxxxxxxxxxxx > --- > drivers/hwmon/cros_ec_hwmon.c | 130 ++++++++++++++++++++++++++++++++++++++++-- > 1 file changed, 125 insertions(+), 5 deletions(-) > > diff --git a/drivers/hwmon/cros_ec_hwmon.c b/drivers/hwmon/cros_ec_hwmon.c > index 9991c3fa020ac859cbbff29dfb669e53248df885..b118a355f67d7238a6f596cf01a49d5b621b31d6 100644 > --- a/drivers/hwmon/cros_ec_hwmon.c > +++ b/drivers/hwmon/cros_ec_hwmon.c > @@ -21,6 +21,12 @@ struct cros_ec_hwmon_priv { > struct cros_ec_device *cros_ec; > const char *temp_sensor_names[EC_TEMP_SENSOR_ENTRIES + EC_TEMP_SENSOR_B_ENTRIES]; > u8 usable_fans; > + int set_fan_target_rpm_version; > +}; > + > +union ec_params_pwm_set_fan_target_rpm { > + struct ec_params_pwm_set_fan_target_rpm_v0 v0; > + struct ec_params_pwm_set_fan_target_rpm_v1 v1; > }; No need to give this union a name. It is only used once. It doesn't even have to be a union but can be two dedicated on-stack variables. > > static int cros_ec_hwmon_read_fan_speed(struct cros_ec_device *cros_ec, u8 index, u16 *speed) > @@ -36,6 +42,25 @@ static int cros_ec_hwmon_read_fan_speed(struct cros_ec_device *cros_ec, u8 index > return 0; > } > > +static int cros_ec_hwmon_read_fan_target(struct cros_ec_device *cros_ec, > + u8 index, u32 *speed) > +{ > + struct ec_response_pwm_get_fan_rpm r; > + int ret; > + > + // Currently only supports reading the first fan. > + if (index > 0) > + return -EOPNOTSUPP; This needs to be checked in is_visible(), not here. (Or only support v1 and not have the restriction) > + > + ret = cros_ec_cmd(cros_ec, 0, EC_CMD_PWM_GET_FAN_TARGET_RPM, NULL, 0, > + &r, sizeof(r)); > + if (ret < 0) > + return ret; > + > + *speed = r.rpm; Comments from v1 still apply. > + return 0; > +} > + > static int cros_ec_hwmon_read_temp(struct cros_ec_device *cros_ec, u8 index, u8 *temp) > { > unsigned int offset; > @@ -52,6 +77,49 @@ static int cros_ec_hwmon_read_temp(struct cros_ec_device *cros_ec, u8 index, u8 > return 0; > } > > +static int cros_ec_hwmon_set_fan_rpm(struct cros_ec_device *cros_ec, > + int version, u8 index, u16 val) > +{ > + union ec_params_pwm_set_fan_target_rpm req; > + int req_size; > + int ret; > + > + if (version == 0) { > + if (index != 0) > + dev_warn( > + cros_ec->dev, > + "v0 only supports setting all fan to same RPM (cannot just set idx %d), set all to %d\n", > + index, val); How important is v0 in general? > + > + req_size = sizeof(req.v0); > + req.v0.rpm = val; > + } else if (version == 1) { > + req_size = sizeof(req.v1); > + req.v1.rpm = val; > + req.v1.fan_idx = index; > + } else > + return -EOPNOTSUPP; If braces are used in the other branches, also use braces here. Also this case can never happen, remove it. > + > + ret = cros_ec_cmd(cros_ec, version, EC_CMD_PWM_SET_FAN_TARGET_RPM, &req, > + req_size, NULL, 0); > + if (ret < 0) > + return ret; > + return 0; > +} > + > +static int cros_ec_hwmon_write_fan(struct cros_ec_hwmon_priv *priv, u32 attr, > + int channel, long rpm) > +{ > + switch (attr) { > + case hwmon_fan_target: > + return cros_ec_hwmon_set_fan_rpm( > + priv->cros_ec, priv->set_fan_target_rpm_version, > + channel, rpm); > + default: > + return -EOPNOTSUPP; > + } > +} > + > static bool cros_ec_hwmon_is_error_fan(u16 speed) > { > return speed == EC_FAN_SPEED_NOT_PRESENT || speed == EC_FAN_SPEED_STALLED; > @@ -75,6 +143,7 @@ static int cros_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type, > { > struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev); > int ret = -EOPNOTSUPP; > + u32 target_rpm; > u16 speed; > u8 temp; > > @@ -91,6 +160,11 @@ static int cros_ec_hwmon_read(struct device *dev, enum hwmon_sensor_types type, > ret = cros_ec_hwmon_read_fan_speed(priv->cros_ec, channel, &speed); > if (ret == 0) > *val = cros_ec_hwmon_is_error_fan(speed); > + } else if (attr == hwmon_fan_target) { > + ret = cros_ec_hwmon_read_fan_target( > + priv->cros_ec, channel, &target_rpm); > + if (ret == 0) > + *val = target_rpm; > } > } else if (type == hwmon_temp) { > if (attr == hwmon_temp_input) { > @@ -130,8 +204,15 @@ static umode_t cros_ec_hwmon_is_visible(const void *data, enum hwmon_sensor_type > const struct cros_ec_hwmon_priv *priv = data; > > if (type == hwmon_fan) { > - if (priv->usable_fans & BIT(channel)) > + if (!(priv->usable_fans & BIT(channel))) > + return 0; > + > + switch (attr) { > + case hwmon_fan_target: > + return 0644; > + default: > return 0444; > + } > } else if (type == hwmon_temp) { > if (priv->temp_sensor_names[channel]) > return 0444; > @@ -140,13 +221,26 @@ static umode_t cros_ec_hwmon_is_visible(const void *data, enum hwmon_sensor_type > return 0; > } > > +static int cros_ec_hwmon_write(struct device *dev, enum hwmon_sensor_types type, > + u32 attr, int channel, long val) > +{ > + struct cros_ec_hwmon_priv *priv = dev_get_drvdata(dev); > + > + switch (type) { > + case hwmon_fan: > + return cros_ec_hwmon_write_fan(priv, attr, channel, val); > + default: > + return -EOPNOTSUPP; > + } > +} > + > static const struct hwmon_channel_info * const cros_ec_hwmon_info[] = { > HWMON_CHANNEL_INFO(chip, HWMON_C_REGISTER_TZ), > HWMON_CHANNEL_INFO(fan, > - HWMON_F_INPUT | HWMON_F_FAULT, > - HWMON_F_INPUT | HWMON_F_FAULT, > - HWMON_F_INPUT | HWMON_F_FAULT, > - HWMON_F_INPUT | HWMON_F_FAULT), > + HWMON_F_INPUT | HWMON_F_FAULT | HWMON_F_TARGET, > + HWMON_F_INPUT | HWMON_F_FAULT | HWMON_F_TARGET, > + HWMON_F_INPUT | HWMON_F_FAULT | HWMON_F_TARGET, > + HWMON_F_INPUT | HWMON_F_FAULT | HWMON_F_TARGET), > HWMON_CHANNEL_INFO(temp, > HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_LABEL, > HWMON_T_INPUT | HWMON_T_FAULT | HWMON_T_LABEL, > @@ -178,6 +272,7 @@ static const struct hwmon_channel_info * const cros_ec_hwmon_info[] = { > static const struct hwmon_ops cros_ec_hwmon_ops = { > .read = cros_ec_hwmon_read, > .read_string = cros_ec_hwmon_read_string, > + .write = cros_ec_hwmon_write, > .is_visible = cros_ec_hwmon_is_visible, > }; > > @@ -233,6 +328,27 @@ static void cros_ec_hwmon_probe_fans(struct cros_ec_hwmon_priv *priv) > } > } > > +static int cros_ec_hwmon_probe_fan_target_cmd_version(struct cros_ec_hwmon_priv *priv) > +{ > + struct ec_params_get_cmd_versions_v1 params = { > + .cmd = EC_CMD_PWM_SET_FAN_TARGET_RPM, > + }; > + struct ec_response_get_cmd_versions response; > + int ret; > + > + ret = cros_ec_cmd(priv->cros_ec, 1, EC_CMD_GET_CMD_VERSIONS, ¶ms, > + sizeof(params), &response, sizeof(response)); cros_ec_get_cmd_versions(). > + if (ret < 0) { > + dev_err(priv->cros_ec->dev, > + "error getting target fan RPM set command version: %d\n", ret); > + return ret; > + } If neither v0 nor v1 are supported, this will completely prevent the driver from being probed. Is this intentional? > + > + priv->set_fan_target_rpm_version = (response.version_mask & EC_VER_MASK(1)) ? 1 : 0; > + > + return 0; > +} > + > static int cros_ec_hwmon_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > @@ -260,6 +376,10 @@ static int cros_ec_hwmon_probe(struct platform_device *pdev) > cros_ec_hwmon_probe_temp_sensors(dev, priv, thermal_version); > cros_ec_hwmon_probe_fans(priv); > > + ret = cros_ec_hwmon_probe_fan_target_cmd_version(priv); > + if (ret < 0) > + return ret; > + > hwmon_dev = devm_hwmon_device_register_with_info(dev, "cros_ec", priv, > &cros_ec_hwmon_chip_info, NULL); > > > --- > base-commit: 80e54e84911a923c40d7bee33a34c1b4be148d7a > change-id: 20250313-extend_ec_hwmon_fan-a5f59aab2cb3 > > Best regards, > -- > Sung-Chi Li <lschyi@xxxxxxxxxxxx> >
