| Message ID | 20210526154022.3223012-5-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | hwmon: (max31790) Fixes and improvements | expand |
fanX_enable works fine. Tested-by: Václav Kubernát <kubernat@cesnet.cz> st 26. 5. 2021 v 17:40 odesílatel Guenter Roeck <linux@roeck-us.net> napsal: > > Since pwmX_enable is now fixed and only handles pwm support instead > of also enabling/disabling fan tachometers, we need an explicit means > to do that. > > For fan channels 7..12, display the enable status if the channel > is configured for fan speed reporting. The displayed status matches > the value of the companion channel but is read-only. > > Cc: Jan Kundrát <jan.kundrat@cesnet.cz> > Cc: Václav Kubernát <kubernat@cesnet.cz> > Signed-off-by: Guenter Roeck <linux@roeck-us.net> > --- > Documentation/hwmon/max31790.rst | 3 ++ > drivers/hwmon/max31790.c | 55 ++++++++++++++++++++++++-------- > 2 files changed, 45 insertions(+), 13 deletions(-) > > diff --git a/Documentation/hwmon/max31790.rst b/Documentation/hwmon/max31790.rst > index 7b097c3b9b90..b43aa32813fd 100644 > --- a/Documentation/hwmon/max31790.rst > +++ b/Documentation/hwmon/max31790.rst > @@ -35,6 +35,9 @@ Sysfs entries > ------------- > > ================== === ======================================================= > +fan[1-12]_enable RW 0=disable fan speed monitoring, 1=enable fan speed monitoring > + The value is RO for companion channels (7-12). For those > + channels, the value matches the value of the primary channel. > fan[1-12]_input RO fan tachometer speed in RPM > fan[1-12]_fault RO fan experienced fault > fan[1-6]_target RW desired fan speed in RPM > diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c > index 67677c437768..19651feb40fb 100644 > --- a/drivers/hwmon/max31790.c > +++ b/drivers/hwmon/max31790.c > @@ -170,6 +170,9 @@ static int max31790_read_fan(struct device *dev, u32 attr, int channel, > return PTR_ERR(data); > > switch (attr) { > + case hwmon_fan_enable: > + *val = !!(data->fan_config[channel % NR_CHANNEL] & MAX31790_FAN_CFG_TACH_INPUT_EN); > + return 0; > case hwmon_fan_input: > sr = get_tach_period(data->fan_dynamics[channel % NR_CHANNEL]); > rpm = RPM_FROM_REG(data->tach[channel], sr); > @@ -195,12 +198,32 @@ static int max31790_write_fan(struct device *dev, u32 attr, int channel, > struct i2c_client *client = data->client; > int target_count; > int err = 0; > - u8 bits; > + u8 bits, config; > int sr; > > mutex_lock(&data->update_lock); > > switch (attr) { > + case hwmon_fan_enable: > + config = data->fan_config[channel]; > + if (val == 0) { > + /* Disabling TACH_INPUT_EN has no effect in RPM_MODE */ > + if (!(config & MAX31790_FAN_CFG_RPM_MODE)) > + config &= ~MAX31790_FAN_CFG_TACH_INPUT_EN; > + } else if (val == 1) { > + config |= MAX31790_FAN_CFG_TACH_INPUT_EN; > + } else { > + err = -EINVAL; > + break; > + } > + if (config != data->fan_config[channel]) { > + err = i2c_smbus_write_byte_data(client, > + MAX31790_REG_FAN_CONFIG(channel), > + config); > + if (!err) > + data->fan_config[channel] = config; > + } > + break; > case hwmon_fan_target: > val = clamp_val(val, FAN_RPM_MIN, FAN_RPM_MAX); > bits = bits_for_tach_period(val); > @@ -240,6 +263,12 @@ static umode_t max31790_fan_is_visible(const void *_data, u32 attr, int channel) > u8 fan_config = data->fan_config[channel % NR_CHANNEL]; > > switch (attr) { > + case hwmon_fan_enable: > + if (channel < NR_CHANNEL) > + return 0644; > + if (fan_config & MAX31790_FAN_CFG_TACH_INPUT) > + return 0444; > + return 0; > case hwmon_fan_input: > case hwmon_fan_fault: > if (channel < NR_CHANNEL || > @@ -404,18 +433,18 @@ static umode_t max31790_is_visible(const void *data, > > static const struct hwmon_channel_info *max31790_info[] = { > HWMON_CHANNEL_INFO(fan, > - HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT, > - HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT, > - HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT, > - HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT, > - HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT, > - HWMON_F_INPUT | HWMON_F_TARGET | 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_INPUT | HWMON_F_FAULT, > - HWMON_F_INPUT | HWMON_F_FAULT), > + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT, > + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT, > + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT, > + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT, > + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT, > + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT, > + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT, > + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT, > + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT, > + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT, > + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT, > + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT), > HWMON_CHANNEL_INFO(pwm, > HWMON_PWM_INPUT | HWMON_PWM_ENABLE, > HWMON_PWM_INPUT | HWMON_PWM_ENABLE, > -- > 2.25.1 >
On středa 26. května 2021 17:40:19 CEST, Guenter Roeck wrote: > Since pwmX_enable is now fixed and only handles pwm support instead > of also enabling/disabling fan tachometers, we need an explicit means > to do that. > > For fan channels 7..12, display the enable status if the channel > is configured for fan speed reporting. The displayed status matches > the value of the companion channel but is read-only. This phrasing is confusing to me. That's once again that "configured to" which in this context doesn't refer to the kernel, but to an initial config of the chip. I suggest the following: Fan channels 7..12 are only available when the chip has been configured with PWM output N-6 disabled. This configuration is outside of scope of the kernel. The displayed status matches the value of the companion channel but is read-only. > +fan[1-12]_enable RW 0=disable fan speed monitoring, > 1=enable fan speed monitoring > + The value is RO for companion channels > (7-12). For those > + channels, the value matches the value > of the primary channel. I realize that it probably doesn't belong to this patch because it affects the other fan_* files, but the docs would be improved by something like: Tachometer inputs monitor fan tachometer logic outputs for precise (+/-1%) monitoring and control of fan RPM as well as detection of fan failure. Six pins are dedicated tachometer inputs. Any of the six PWM outputs can -also be configured to serve as tachometer inputs. +also be reconfigured to serve as tachometer inputs by the firmware. The +kernel will respect the initial configuration of the chip. Want an extra patch on top of this series? > + case hwmon_fan_enable: > + config = data->fan_config[channel]; > + if (val == 0) { > + /* Disabling TACH_INPUT_EN has no effect in RPM_MODE */ > + if (!(config & MAX31790_FAN_CFG_RPM_MODE)) > + config &= ~MAX31790_FAN_CFG_TACH_INPUT_EN; This means that a "nonsensical" write from userspace will be silently ignored, doesn't it? I think it should return an error instead. Cheers, Jan
On Wed, Jun 02, 2021 at 03:04:35PM +0200, Jan Kundrát wrote: > On středa 26. května 2021 17:40:19 CEST, Guenter Roeck wrote: > > Since pwmX_enable is now fixed and only handles pwm support instead > > of also enabling/disabling fan tachometers, we need an explicit means > > to do that. > > > > For fan channels 7..12, display the enable status if the channel > > is configured for fan speed reporting. The displayed status matches > > the value of the companion channel but is read-only. > > This phrasing is confusing to me. That's once again that "configured to" > which in this context doesn't refer to the kernel, but to an initial config > of the chip. I suggest the following: > > Fan channels 7..12 are only available when the chip has been configured with > PWM output N-6 disabled. This configuration is outside of scope of the > kernel. The displayed status matches the value of the companion channel but > is read-only. > > > +fan[1-12]_enable RW 0=disable fan speed monitoring, 1=enable fan > > speed monitoring > > + The value is RO for companion channels (7-12). > > For those > > + channels, the value matches the value of the > > primary channel. > > I realize that it probably doesn't belong to this patch because it affects > the other fan_* files, but the docs would be improved by something like: > > Tachometer inputs monitor fan tachometer logic outputs for precise (+/-1%) > monitoring and control of fan RPM as well as detection of fan failure. > Six pins are dedicated tachometer inputs. Any of the six PWM outputs can > -also be configured to serve as tachometer inputs. > +also be reconfigured to serve as tachometer inputs by the firmware. The > +kernel will respect the initial configuration of the chip. > "Precise (+/-1%)" sounds like chip advertising, which I'd rather avoid. > Want an extra patch on top of this series? > > > + case hwmon_fan_enable: > > + config = data->fan_config[channel]; > > + if (val == 0) { > > + /* Disabling TACH_INPUT_EN has no effect in RPM_MODE */ > > + if (!(config & MAX31790_FAN_CFG_RPM_MODE)) > > + config &= ~MAX31790_FAN_CFG_TACH_INPUT_EN; > > This means that a "nonsensical" write from userspace will be silently > ignored, doesn't it? I think it should return an error instead. > Trade-off between confusing users and trying to match the ABI with somewhat odd chip capabilities. The above isn't exactly specified; it is the result of trial and error (and a reason why the configuration register must be treated as volatile when using regmap). One can argue one way or another. For now I'd rather keep the code as is because I am away from the evaluation board for the next few weeks and won't be able to test any functional changes until I am back. Given the complexity of the chip and its sometimes odd behavior I'll want to be able to do that kind of testing. Is this important for you ? If so, we can move forward with patches 1-3 of the series and leave this and subsequent patches for later. Thanks, Guenter
diff --git a/Documentation/hwmon/max31790.rst b/Documentation/hwmon/max31790.rst index 7b097c3b9b90..b43aa32813fd 100644 --- a/Documentation/hwmon/max31790.rst +++ b/Documentation/hwmon/max31790.rst @@ -35,6 +35,9 @@ Sysfs entries ------------- ================== === ======================================================= +fan[1-12]_enable RW 0=disable fan speed monitoring, 1=enable fan speed monitoring + The value is RO for companion channels (7-12). For those + channels, the value matches the value of the primary channel. fan[1-12]_input RO fan tachometer speed in RPM fan[1-12]_fault RO fan experienced fault fan[1-6]_target RW desired fan speed in RPM diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c index 67677c437768..19651feb40fb 100644 --- a/drivers/hwmon/max31790.c +++ b/drivers/hwmon/max31790.c @@ -170,6 +170,9 @@ static int max31790_read_fan(struct device *dev, u32 attr, int channel, return PTR_ERR(data); switch (attr) { + case hwmon_fan_enable: + *val = !!(data->fan_config[channel % NR_CHANNEL] & MAX31790_FAN_CFG_TACH_INPUT_EN); + return 0; case hwmon_fan_input: sr = get_tach_period(data->fan_dynamics[channel % NR_CHANNEL]); rpm = RPM_FROM_REG(data->tach[channel], sr); @@ -195,12 +198,32 @@ static int max31790_write_fan(struct device *dev, u32 attr, int channel, struct i2c_client *client = data->client; int target_count; int err = 0; - u8 bits; + u8 bits, config; int sr; mutex_lock(&data->update_lock); switch (attr) { + case hwmon_fan_enable: + config = data->fan_config[channel]; + if (val == 0) { + /* Disabling TACH_INPUT_EN has no effect in RPM_MODE */ + if (!(config & MAX31790_FAN_CFG_RPM_MODE)) + config &= ~MAX31790_FAN_CFG_TACH_INPUT_EN; + } else if (val == 1) { + config |= MAX31790_FAN_CFG_TACH_INPUT_EN; + } else { + err = -EINVAL; + break; + } + if (config != data->fan_config[channel]) { + err = i2c_smbus_write_byte_data(client, + MAX31790_REG_FAN_CONFIG(channel), + config); + if (!err) + data->fan_config[channel] = config; + } + break; case hwmon_fan_target: val = clamp_val(val, FAN_RPM_MIN, FAN_RPM_MAX); bits = bits_for_tach_period(val); @@ -240,6 +263,12 @@ static umode_t max31790_fan_is_visible(const void *_data, u32 attr, int channel) u8 fan_config = data->fan_config[channel % NR_CHANNEL]; switch (attr) { + case hwmon_fan_enable: + if (channel < NR_CHANNEL) + return 0644; + if (fan_config & MAX31790_FAN_CFG_TACH_INPUT) + return 0444; + return 0; case hwmon_fan_input: case hwmon_fan_fault: if (channel < NR_CHANNEL || @@ -404,18 +433,18 @@ static umode_t max31790_is_visible(const void *data, static const struct hwmon_channel_info *max31790_info[] = { HWMON_CHANNEL_INFO(fan, - HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT, - HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT, - HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT, - HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT, - HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT, - HWMON_F_INPUT | HWMON_F_TARGET | 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_INPUT | HWMON_F_FAULT, - HWMON_F_INPUT | HWMON_F_FAULT), + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT, + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT, + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT, + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT, + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT, + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_TARGET | HWMON_F_FAULT, + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT, + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT, + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT, + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT, + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT, + HWMON_F_ENABLE | HWMON_F_INPUT | HWMON_F_FAULT), HWMON_CHANNEL_INFO(pwm, HWMON_PWM_INPUT | HWMON_PWM_ENABLE, HWMON_PWM_INPUT | HWMON_PWM_ENABLE,
Since pwmX_enable is now fixed and only handles pwm support instead of also enabling/disabling fan tachometers, we need an explicit means to do that. For fan channels 7..12, display the enable status if the channel is configured for fan speed reporting. The displayed status matches the value of the companion channel but is read-only. Cc: Jan Kundrát <jan.kundrat@cesnet.cz> Cc: Václav Kubernát <kubernat@cesnet.cz> Signed-off-by: Guenter Roeck <linux@roeck-us.net> --- Documentation/hwmon/max31790.rst | 3 ++ drivers/hwmon/max31790.c | 55 ++++++++++++++++++++++++-------- 2 files changed, 45 insertions(+), 13 deletions(-)