| Message ID | 20220829024351.2415147-1-justinledford@google.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hwmon: (max31790) add fanN_enable | expand |
On Mon, Aug 29, 2022 at 02:43:51AM +0000, Justin Ledford wrote: > The MAX31790 has a tach input enable bit in each fan's configuration > register. This is only enabled by the driver if RPM mode is selected, > but the driver doesn't provide a way to independently enable tachometer > input regardless of the regulator mode. > > By adding the fanN_enable sysfs files, we can decouple the tach input > from the regulator mode. Also update the documentation. > > Signed-off-by: Justin Ledford <justinledford@google.com> > --- > Documentation/hwmon/max31790.rst | 1 + > drivers/hwmon/max31790.c | 44 +++++++++++++++++++++++++++----- > 2 files changed, 38 insertions(+), 7 deletions(-) > > diff --git a/Documentation/hwmon/max31790.rst b/Documentation/hwmon/max31790.rst > index 7b097c3b9b90..33c5c7330efc 100644 > --- a/Documentation/hwmon/max31790.rst > +++ b/Documentation/hwmon/max31790.rst > @@ -38,6 +38,7 @@ Sysfs entries > 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 > +fan[1-6]_enable RW enable or disable the tachometer input > pwm[1-6]_enable RW regulator mode, 0=disabled (duty cycle=0%), 1=manual mode, 2=rpm mode > pwm[1-6] RW read: current pwm duty cycle, > write: target pwm duty cycle (0-255) > diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c > index 7e9362f6dc29..3ae02be4b41e 100644 > --- a/drivers/hwmon/max31790.c > +++ b/drivers/hwmon/max31790.c > @@ -118,6 +118,12 @@ static struct max31790_data *max31790_update_device(struct device *dev) > goto abort; > data->target_count[i] = rv; > } > + > + rv = i2c_smbus_read_byte_data(client, > + MAX31790_REG_FAN_CONFIG(i)); > + if (rv < 0) > + goto abort; > + data->fan_config[i] = rv; Why is this needed ? Guenter > } > > data->last_updated = jiffies; > @@ -202,6 +208,9 @@ static int max31790_read_fan(struct device *dev, u32 attr, int channel, > } > mutex_unlock(&data->update_lock); > return 0; > + case hwmon_fan_enable: > + *val = !!(data->fan_config[channel] & MAX31790_FAN_CFG_TACH_INPUT_EN); > + return 0; > default: > return -EOPNOTSUPP; > } > @@ -214,7 +223,7 @@ 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, fan_config; > int sr; > > mutex_lock(&data->update_lock); > @@ -243,6 +252,23 @@ static int max31790_write_fan(struct device *dev, u32 attr, int channel, > MAX31790_REG_TARGET_COUNT(channel), > data->target_count[channel]); > break; > + case hwmon_fan_enable: > + fan_config = data->fan_config[channel]; > + if (val == 0) { > + fan_config &= ~MAX31790_FAN_CFG_TACH_INPUT_EN; > + } else if (val == 1) { > + fan_config |= MAX31790_FAN_CFG_TACH_INPUT_EN; > + } else { > + err = -EINVAL; > + break; > + } > + if (fan_config != data->fan_config[channel]) { > + err = i2c_smbus_write_byte_data(client, MAX31790_REG_FAN_CONFIG(channel), > + fan_config); > + if (!err) > + data->fan_config[channel] = fan_config; > + } > + break; > default: > err = -EOPNOTSUPP; > break; > @@ -270,6 +296,10 @@ static umode_t max31790_fan_is_visible(const void *_data, u32 attr, int channel) > !(fan_config & MAX31790_FAN_CFG_TACH_INPUT)) > return 0644; > return 0; > + case hwmon_fan_enable: > + if (channel < NR_CHANNEL) > + return 0644; > + return 0; > default: > return 0; > } > @@ -423,12 +453,12 @@ 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_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_INPUT | HWMON_F_FAULT, > HWMON_F_INPUT | HWMON_F_FAULT, > -- > 2.37.2.672.g94769d06f0-goog >
The tach input isn't enabled in the device by default. So the only way to start using the fan input sensors is to set the regulator mode through the driver to RPM mode and then back to whatever mode you actually want to use. The I2C interface to the device doesn't couple the tach input to the regulator mode so I don't think it makes sense for the driver to do this either. On Mon, Aug 29, 2022 at 6:20 AM Guenter Roeck <linux@roeck-us.net> wrote: > > On Mon, Aug 29, 2022 at 02:43:51AM +0000, Justin Ledford wrote: > > The MAX31790 has a tach input enable bit in each fan's configuration > > register. This is only enabled by the driver if RPM mode is selected, > > but the driver doesn't provide a way to independently enable tachometer > > input regardless of the regulator mode. > > > > By adding the fanN_enable sysfs files, we can decouple the tach input > > from the regulator mode. Also update the documentation. > > > > Signed-off-by: Justin Ledford <justinledford@google.com> > > --- > > Documentation/hwmon/max31790.rst | 1 + > > drivers/hwmon/max31790.c | 44 +++++++++++++++++++++++++++----- > > 2 files changed, 38 insertions(+), 7 deletions(-) > > > > diff --git a/Documentation/hwmon/max31790.rst b/Documentation/hwmon/max31790.rst > > index 7b097c3b9b90..33c5c7330efc 100644 > > --- a/Documentation/hwmon/max31790.rst > > +++ b/Documentation/hwmon/max31790.rst > > @@ -38,6 +38,7 @@ Sysfs entries > > 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 > > +fan[1-6]_enable RW enable or disable the tachometer input > > pwm[1-6]_enable RW regulator mode, 0=disabled (duty cycle=0%), 1=manual mode, 2=rpm mode > > pwm[1-6] RW read: current pwm duty cycle, > > write: target pwm duty cycle (0-255) > > diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c > > index 7e9362f6dc29..3ae02be4b41e 100644 > > --- a/drivers/hwmon/max31790.c > > +++ b/drivers/hwmon/max31790.c > > @@ -118,6 +118,12 @@ static struct max31790_data *max31790_update_device(struct device *dev) > > goto abort; > > data->target_count[i] = rv; > > } > > + > > + rv = i2c_smbus_read_byte_data(client, > > + MAX31790_REG_FAN_CONFIG(i)); > > + if (rv < 0) > > + goto abort; > > + data->fan_config[i] = rv; > > Why is this needed ? > > Guenter > > > } > > > > data->last_updated = jiffies; > > @@ -202,6 +208,9 @@ static int max31790_read_fan(struct device *dev, u32 attr, int channel, > > } > > mutex_unlock(&data->update_lock); > > return 0; > > + case hwmon_fan_enable: > > + *val = !!(data->fan_config[channel] & MAX31790_FAN_CFG_TACH_INPUT_EN); > > + return 0; > > default: > > return -EOPNOTSUPP; > > } > > @@ -214,7 +223,7 @@ 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, fan_config; > > int sr; > > > > mutex_lock(&data->update_lock); > > @@ -243,6 +252,23 @@ static int max31790_write_fan(struct device *dev, u32 attr, int channel, > > MAX31790_REG_TARGET_COUNT(channel), > > data->target_count[channel]); > > break; > > + case hwmon_fan_enable: > > + fan_config = data->fan_config[channel]; > > + if (val == 0) { > > + fan_config &= ~MAX31790_FAN_CFG_TACH_INPUT_EN; > > + } else if (val == 1) { > > + fan_config |= MAX31790_FAN_CFG_TACH_INPUT_EN; > > + } else { > > + err = -EINVAL; > > + break; > > + } > > + if (fan_config != data->fan_config[channel]) { > > + err = i2c_smbus_write_byte_data(client, MAX31790_REG_FAN_CONFIG(channel), > > + fan_config); > > + if (!err) > > + data->fan_config[channel] = fan_config; > > + } > > + break; > > default: > > err = -EOPNOTSUPP; > > break; > > @@ -270,6 +296,10 @@ static umode_t max31790_fan_is_visible(const void *_data, u32 attr, int channel) > > !(fan_config & MAX31790_FAN_CFG_TACH_INPUT)) > > return 0644; > > return 0; > > + case hwmon_fan_enable: > > + if (channel < NR_CHANNEL) > > + return 0644; > > + return 0; > > default: > > return 0; > > } > > @@ -423,12 +453,12 @@ 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_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_INPUT | HWMON_F_FAULT, > > HWMON_F_INPUT | HWMON_F_FAULT, > > -- > > 2.37.2.672.g94769d06f0-goog > >
On Mon, Aug 29, 2022 at 08:09:21AM -0700, Justin Ledford wrote: > The tach input isn't enabled in the device by default. So the only way > to start using the fan input sensors is to set the regulator mode > through the driver to RPM mode and then back to whatever mode you > actually want to use. The I2C interface to the device doesn't couple > the tach input to the regulator mode so I don't think it makes sense > for the driver to do this either. > Please don't top-post. The above does not answer my question why fan_config[] wound need to be updated repeatedly. Guenter > On Mon, Aug 29, 2022 at 6:20 AM Guenter Roeck <linux@roeck-us.net> wrote: > > > > On Mon, Aug 29, 2022 at 02:43:51AM +0000, Justin Ledford wrote: > > > The MAX31790 has a tach input enable bit in each fan's configuration > > > register. This is only enabled by the driver if RPM mode is selected, > > > but the driver doesn't provide a way to independently enable tachometer > > > input regardless of the regulator mode. > > > > > > By adding the fanN_enable sysfs files, we can decouple the tach input > > > from the regulator mode. Also update the documentation. > > > > > > Signed-off-by: Justin Ledford <justinledford@google.com> > > > --- > > > Documentation/hwmon/max31790.rst | 1 + > > > drivers/hwmon/max31790.c | 44 +++++++++++++++++++++++++++----- > > > 2 files changed, 38 insertions(+), 7 deletions(-) > > > > > > diff --git a/Documentation/hwmon/max31790.rst b/Documentation/hwmon/max31790.rst > > > index 7b097c3b9b90..33c5c7330efc 100644 > > > --- a/Documentation/hwmon/max31790.rst > > > +++ b/Documentation/hwmon/max31790.rst > > > @@ -38,6 +38,7 @@ Sysfs entries > > > 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 > > > +fan[1-6]_enable RW enable or disable the tachometer input > > > pwm[1-6]_enable RW regulator mode, 0=disabled (duty cycle=0%), 1=manual mode, 2=rpm mode > > > pwm[1-6] RW read: current pwm duty cycle, > > > write: target pwm duty cycle (0-255) > > > diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c > > > index 7e9362f6dc29..3ae02be4b41e 100644 > > > --- a/drivers/hwmon/max31790.c > > > +++ b/drivers/hwmon/max31790.c > > > @@ -118,6 +118,12 @@ static struct max31790_data *max31790_update_device(struct device *dev) > > > goto abort; > > > data->target_count[i] = rv; > > > } > > > + > > > + rv = i2c_smbus_read_byte_data(client, > > > + MAX31790_REG_FAN_CONFIG(i)); > > > + if (rv < 0) > > > + goto abort; > > > + data->fan_config[i] = rv; > > > > Why is this needed ? > > > > Guenter > > > > > } > > > > > > data->last_updated = jiffies; > > > @@ -202,6 +208,9 @@ static int max31790_read_fan(struct device *dev, u32 attr, int channel, > > > } > > > mutex_unlock(&data->update_lock); > > > return 0; > > > + case hwmon_fan_enable: > > > + *val = !!(data->fan_config[channel] & MAX31790_FAN_CFG_TACH_INPUT_EN); > > > + return 0; > > > default: > > > return -EOPNOTSUPP; > > > } > > > @@ -214,7 +223,7 @@ 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, fan_config; > > > int sr; > > > > > > mutex_lock(&data->update_lock); > > > @@ -243,6 +252,23 @@ static int max31790_write_fan(struct device *dev, u32 attr, int channel, > > > MAX31790_REG_TARGET_COUNT(channel), > > > data->target_count[channel]); > > > break; > > > + case hwmon_fan_enable: > > > + fan_config = data->fan_config[channel]; > > > + if (val == 0) { > > > + fan_config &= ~MAX31790_FAN_CFG_TACH_INPUT_EN; > > > + } else if (val == 1) { > > > + fan_config |= MAX31790_FAN_CFG_TACH_INPUT_EN; > > > + } else { > > > + err = -EINVAL; > > > + break; > > > + } > > > + if (fan_config != data->fan_config[channel]) { > > > + err = i2c_smbus_write_byte_data(client, MAX31790_REG_FAN_CONFIG(channel), > > > + fan_config); > > > + if (!err) > > > + data->fan_config[channel] = fan_config; > > > + } > > > + break; > > > default: > > > err = -EOPNOTSUPP; > > > break; > > > @@ -270,6 +296,10 @@ static umode_t max31790_fan_is_visible(const void *_data, u32 attr, int channel) > > > !(fan_config & MAX31790_FAN_CFG_TACH_INPUT)) > > > return 0644; > > > return 0; > > > + case hwmon_fan_enable: > > > + if (channel < NR_CHANNEL) > > > + return 0644; > > > + return 0; > > > default: > > > return 0; > > > } > > > @@ -423,12 +453,12 @@ 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_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_INPUT | HWMON_F_FAULT, > > > HWMON_F_INPUT | HWMON_F_FAULT, > > > -- > > > 2.37.2.672.g94769d06f0-goog > > >
On Mon, Aug 29, 2022 at 9:11 AM Guenter Roeck <linux@roeck-us.net> wrote: > > On Mon, Aug 29, 2022 at 08:09:21AM -0700, Justin Ledford wrote: > > The tach input isn't enabled in the device by default. So the only way > > to start using the fan input sensors is to set the regulator mode > > through the driver to RPM mode and then back to whatever mode you > > actually want to use. The I2C interface to the device doesn't couple > > the tach input to the regulator mode so I don't think it makes sense > > for the driver to do this either. > > > Please don't top-post. > > The above does not answer my question why fan_config[] wound need to > be updated repeatedly. > > Guenter > > > On Mon, Aug 29, 2022 at 6:20 AM Guenter Roeck <linux@roeck-us.net> wrote: > > > > > > On Mon, Aug 29, 2022 at 02:43:51AM +0000, Justin Ledford wrote: > > > > The MAX31790 has a tach input enable bit in each fan's configuration > > > > register. This is only enabled by the driver if RPM mode is selected, > > > > but the driver doesn't provide a way to independently enable tachometer > > > > input regardless of the regulator mode. > > > > > > > > By adding the fanN_enable sysfs files, we can decouple the tach input > > > > from the regulator mode. Also update the documentation. > > > > > > > > Signed-off-by: Justin Ledford <justinledford@google.com> > > > > --- > > > > Documentation/hwmon/max31790.rst | 1 + > > > > drivers/hwmon/max31790.c | 44 +++++++++++++++++++++++++++----- > > > > 2 files changed, 38 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/Documentation/hwmon/max31790.rst b/Documentation/hwmon/max31790.rst > > > > index 7b097c3b9b90..33c5c7330efc 100644 > > > > --- a/Documentation/hwmon/max31790.rst > > > > +++ b/Documentation/hwmon/max31790.rst > > > > @@ -38,6 +38,7 @@ Sysfs entries > > > > 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 > > > > +fan[1-6]_enable RW enable or disable the tachometer input > > > > pwm[1-6]_enable RW regulator mode, 0=disabled (duty cycle=0%), 1=manual mode, 2=rpm mode > > > > pwm[1-6] RW read: current pwm duty cycle, > > > > write: target pwm duty cycle (0-255) > > > > diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c > > > > index 7e9362f6dc29..3ae02be4b41e 100644 > > > > --- a/drivers/hwmon/max31790.c > > > > +++ b/drivers/hwmon/max31790.c > > > > @@ -118,6 +118,12 @@ static struct max31790_data *max31790_update_device(struct device *dev) > > > > goto abort; > > > > data->target_count[i] = rv; > > > > } > > > > + > > > > + rv = i2c_smbus_read_byte_data(client, > > > > + MAX31790_REG_FAN_CONFIG(i)); > > > > + if (rv < 0) > > > > + goto abort; > > > > + data->fan_config[i] = rv; > > > > > > Why is this needed ? > > > > > > Guenter > > > This is needed in case the fan_config is changed outside the driver with something like i2ctransfer, so that the driver reports the actual state of the device, rather than the state at the time of the last write originating from the driver. > > > > } > > > > > > > > data->last_updated = jiffies; > > > > @@ -202,6 +208,9 @@ static int max31790_read_fan(struct device *dev, u32 attr, int channel, > > > > } > > > > mutex_unlock(&data->update_lock); > > > > return 0; > > > > + case hwmon_fan_enable: > > > > + *val = !!(data->fan_config[channel] & MAX31790_FAN_CFG_TACH_INPUT_EN); > > > > + return 0; > > > > default: > > > > return -EOPNOTSUPP; > > > > } > > > > @@ -214,7 +223,7 @@ 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, fan_config; > > > > int sr; > > > > > > > > mutex_lock(&data->update_lock); > > > > @@ -243,6 +252,23 @@ static int max31790_write_fan(struct device *dev, u32 attr, int channel, > > > > MAX31790_REG_TARGET_COUNT(channel), > > > > data->target_count[channel]); > > > > break; > > > > + case hwmon_fan_enable: > > > > + fan_config = data->fan_config[channel]; > > > > + if (val == 0) { > > > > + fan_config &= ~MAX31790_FAN_CFG_TACH_INPUT_EN; > > > > + } else if (val == 1) { > > > > + fan_config |= MAX31790_FAN_CFG_TACH_INPUT_EN; > > > > + } else { > > > > + err = -EINVAL; > > > > + break; > > > > + } > > > > + if (fan_config != data->fan_config[channel]) { > > > > + err = i2c_smbus_write_byte_data(client, MAX31790_REG_FAN_CONFIG(channel), > > > > + fan_config); > > > > + if (!err) > > > > + data->fan_config[channel] = fan_config; > > > > + } > > > > + break; > > > > default: > > > > err = -EOPNOTSUPP; > > > > break; > > > > @@ -270,6 +296,10 @@ static umode_t max31790_fan_is_visible(const void *_data, u32 attr, int channel) > > > > !(fan_config & MAX31790_FAN_CFG_TACH_INPUT)) > > > > return 0644; > > > > return 0; > > > > + case hwmon_fan_enable: > > > > + if (channel < NR_CHANNEL) > > > > + return 0644; > > > > + return 0; > > > > default: > > > > return 0; > > > > } > > > > @@ -423,12 +453,12 @@ 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_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_INPUT | HWMON_F_FAULT, > > > > HWMON_F_INPUT | HWMON_F_FAULT, > > > > -- > > > > 2.37.2.672.g94769d06f0-goog > > > >
On 8/29/22 10:15, Justin Ledford wrote: > On Mon, Aug 29, 2022 at 9:11 AM Guenter Roeck <linux@roeck-us.net> wrote: >> >> On Mon, Aug 29, 2022 at 08:09:21AM -0700, Justin Ledford wrote: >>> The tach input isn't enabled in the device by default. So the only way >>> to start using the fan input sensors is to set the regulator mode >>> through the driver to RPM mode and then back to whatever mode you >>> actually want to use. The I2C interface to the device doesn't couple >>> the tach input to the regulator mode so I don't think it makes sense >>> for the driver to do this either. >>> >> Please don't top-post. >> >> The above does not answer my question why fan_config[] wound need to >> be updated repeatedly. >> >> Guenter >> >>> On Mon, Aug 29, 2022 at 6:20 AM Guenter Roeck <linux@roeck-us.net> wrote: >>>> >>>> On Mon, Aug 29, 2022 at 02:43:51AM +0000, Justin Ledford wrote: >>>>> The MAX31790 has a tach input enable bit in each fan's configuration >>>>> register. This is only enabled by the driver if RPM mode is selected, >>>>> but the driver doesn't provide a way to independently enable tachometer >>>>> input regardless of the regulator mode. >>>>> >>>>> By adding the fanN_enable sysfs files, we can decouple the tach input >>>>> from the regulator mode. Also update the documentation. >>>>> >>>>> Signed-off-by: Justin Ledford <justinledford@google.com> >>>>> --- >>>>> Documentation/hwmon/max31790.rst | 1 + >>>>> drivers/hwmon/max31790.c | 44 +++++++++++++++++++++++++++----- >>>>> 2 files changed, 38 insertions(+), 7 deletions(-) >>>>> >>>>> diff --git a/Documentation/hwmon/max31790.rst b/Documentation/hwmon/max31790.rst >>>>> index 7b097c3b9b90..33c5c7330efc 100644 >>>>> --- a/Documentation/hwmon/max31790.rst >>>>> +++ b/Documentation/hwmon/max31790.rst >>>>> @@ -38,6 +38,7 @@ Sysfs entries >>>>> 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 >>>>> +fan[1-6]_enable RW enable or disable the tachometer input >>>>> pwm[1-6]_enable RW regulator mode, 0=disabled (duty cycle=0%), 1=manual mode, 2=rpm mode >>>>> pwm[1-6] RW read: current pwm duty cycle, >>>>> write: target pwm duty cycle (0-255) >>>>> diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c >>>>> index 7e9362f6dc29..3ae02be4b41e 100644 >>>>> --- a/drivers/hwmon/max31790.c >>>>> +++ b/drivers/hwmon/max31790.c >>>>> @@ -118,6 +118,12 @@ static struct max31790_data *max31790_update_device(struct device *dev) >>>>> goto abort; >>>>> data->target_count[i] = rv; >>>>> } >>>>> + >>>>> + rv = i2c_smbus_read_byte_data(client, >>>>> + MAX31790_REG_FAN_CONFIG(i)); >>>>> + if (rv < 0) >>>>> + goto abort; >>>>> + data->fan_config[i] = rv; >>>> >>>> Why is this needed ? >>>> >>>> Guenter >>>> > > This is needed in case the fan_config is changed outside the driver > with something like i2ctransfer, so that the driver reports the actual > state of the device, rather than the state at the time of the last > write originating from the driver. > That is not a concern or valid argument. With such an argument, not a single I2C (or, for that matter, any other) register would be cacheable, and much of the caching code in the kernel would not work. Anyone hacking around any driver in the system would be on their own. One could even argue that such hacking should be undone if possible because it may have severe and unexpected impact on any driver operation. Guenter >>>>> } >>>>> >>>>> data->last_updated = jiffies; >>>>> @@ -202,6 +208,9 @@ static int max31790_read_fan(struct device *dev, u32 attr, int channel, >>>>> } >>>>> mutex_unlock(&data->update_lock); >>>>> return 0; >>>>> + case hwmon_fan_enable: >>>>> + *val = !!(data->fan_config[channel] & MAX31790_FAN_CFG_TACH_INPUT_EN); >>>>> + return 0; >>>>> default: >>>>> return -EOPNOTSUPP; >>>>> } >>>>> @@ -214,7 +223,7 @@ 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, fan_config; >>>>> int sr; >>>>> >>>>> mutex_lock(&data->update_lock); >>>>> @@ -243,6 +252,23 @@ static int max31790_write_fan(struct device *dev, u32 attr, int channel, >>>>> MAX31790_REG_TARGET_COUNT(channel), >>>>> data->target_count[channel]); >>>>> break; >>>>> + case hwmon_fan_enable: >>>>> + fan_config = data->fan_config[channel]; >>>>> + if (val == 0) { >>>>> + fan_config &= ~MAX31790_FAN_CFG_TACH_INPUT_EN; >>>>> + } else if (val == 1) { >>>>> + fan_config |= MAX31790_FAN_CFG_TACH_INPUT_EN; >>>>> + } else { >>>>> + err = -EINVAL; >>>>> + break; >>>>> + } >>>>> + if (fan_config != data->fan_config[channel]) { >>>>> + err = i2c_smbus_write_byte_data(client, MAX31790_REG_FAN_CONFIG(channel), >>>>> + fan_config); >>>>> + if (!err) >>>>> + data->fan_config[channel] = fan_config; >>>>> + } >>>>> + break; >>>>> default: >>>>> err = -EOPNOTSUPP; >>>>> break; >>>>> @@ -270,6 +296,10 @@ static umode_t max31790_fan_is_visible(const void *_data, u32 attr, int channel) >>>>> !(fan_config & MAX31790_FAN_CFG_TACH_INPUT)) >>>>> return 0644; >>>>> return 0; >>>>> + case hwmon_fan_enable: >>>>> + if (channel < NR_CHANNEL) >>>>> + return 0644; >>>>> + return 0; >>>>> default: >>>>> return 0; >>>>> } >>>>> @@ -423,12 +453,12 @@ 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_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_INPUT | HWMON_F_FAULT, >>>>> HWMON_F_INPUT | HWMON_F_FAULT, >>>>> -- >>>>> 2.37.2.672.g94769d06f0-goog >>>>>
On Mon, Aug 29, 2022 at 11:08 AM Guenter Roeck <linux@roeck-us.net> wrote: > > On 8/29/22 10:15, Justin Ledford wrote: > > On Mon, Aug 29, 2022 at 9:11 AM Guenter Roeck <linux@roeck-us.net> wrote: > >> > >> On Mon, Aug 29, 2022 at 08:09:21AM -0700, Justin Ledford wrote: > >>> The tach input isn't enabled in the device by default. So the only way > >>> to start using the fan input sensors is to set the regulator mode > >>> through the driver to RPM mode and then back to whatever mode you > >>> actually want to use. The I2C interface to the device doesn't couple > >>> the tach input to the regulator mode so I don't think it makes sense > >>> for the driver to do this either. > >>> > >> Please don't top-post. > >> > >> The above does not answer my question why fan_config[] wound need to > >> be updated repeatedly. > >> > >> Guenter > >> > >>> On Mon, Aug 29, 2022 at 6:20 AM Guenter Roeck <linux@roeck-us.net> wrote: > >>>> > >>>> On Mon, Aug 29, 2022 at 02:43:51AM +0000, Justin Ledford wrote: > >>>>> The MAX31790 has a tach input enable bit in each fan's configuration > >>>>> register. This is only enabled by the driver if RPM mode is selected, > >>>>> but the driver doesn't provide a way to independently enable tachometer > >>>>> input regardless of the regulator mode. > >>>>> > >>>>> By adding the fanN_enable sysfs files, we can decouple the tach input > >>>>> from the regulator mode. Also update the documentation. > >>>>> > >>>>> Signed-off-by: Justin Ledford <justinledford@google.com> > >>>>> --- > >>>>> Documentation/hwmon/max31790.rst | 1 + > >>>>> drivers/hwmon/max31790.c | 44 +++++++++++++++++++++++++++----- > >>>>> 2 files changed, 38 insertions(+), 7 deletions(-) > >>>>> > >>>>> diff --git a/Documentation/hwmon/max31790.rst b/Documentation/hwmon/max31790.rst > >>>>> index 7b097c3b9b90..33c5c7330efc 100644 > >>>>> --- a/Documentation/hwmon/max31790.rst > >>>>> +++ b/Documentation/hwmon/max31790.rst > >>>>> @@ -38,6 +38,7 @@ Sysfs entries > >>>>> 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 > >>>>> +fan[1-6]_enable RW enable or disable the tachometer input > >>>>> pwm[1-6]_enable RW regulator mode, 0=disabled (duty cycle=0%), 1=manual mode, 2=rpm mode > >>>>> pwm[1-6] RW read: current pwm duty cycle, > >>>>> write: target pwm duty cycle (0-255) > >>>>> diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c > >>>>> index 7e9362f6dc29..3ae02be4b41e 100644 > >>>>> --- a/drivers/hwmon/max31790.c > >>>>> +++ b/drivers/hwmon/max31790.c > >>>>> @@ -118,6 +118,12 @@ static struct max31790_data *max31790_update_device(struct device *dev) > >>>>> goto abort; > >>>>> data->target_count[i] = rv; > >>>>> } > >>>>> + > >>>>> + rv = i2c_smbus_read_byte_data(client, > >>>>> + MAX31790_REG_FAN_CONFIG(i)); > >>>>> + if (rv < 0) > >>>>> + goto abort; > >>>>> + data->fan_config[i] = rv; > >>>> > >>>> Why is this needed ? > >>>> > >>>> Guenter > >>>> > > > > This is needed in case the fan_config is changed outside the driver > > with something like i2ctransfer, so that the driver reports the actual > > state of the device, rather than the state at the time of the last > > write originating from the driver. > > > > That is not a concern or valid argument. With such an argument, > not a single I2C (or, for that matter, any other) register would > be cacheable, and much of the caching code in the kernel would > not work. Anyone hacking around any driver in the system would > be on their own. One could even argue that such hacking should > be undone if possible because it may have severe and unexpected > impact on any driver operation. > > Guenter > Thank you for the feedback, that makes sense. I will send an updated patch with this chunk removed. > >>>>> } > >>>>> > >>>>> data->last_updated = jiffies; > >>>>> @@ -202,6 +208,9 @@ static int max31790_read_fan(struct device *dev, u32 attr, int channel, > >>>>> } > >>>>> mutex_unlock(&data->update_lock); > >>>>> return 0; > >>>>> + case hwmon_fan_enable: > >>>>> + *val = !!(data->fan_config[channel] & MAX31790_FAN_CFG_TACH_INPUT_EN); > >>>>> + return 0; > >>>>> default: > >>>>> return -EOPNOTSUPP; > >>>>> } > >>>>> @@ -214,7 +223,7 @@ 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, fan_config; > >>>>> int sr; > >>>>> > >>>>> mutex_lock(&data->update_lock); > >>>>> @@ -243,6 +252,23 @@ static int max31790_write_fan(struct device *dev, u32 attr, int channel, > >>>>> MAX31790_REG_TARGET_COUNT(channel), > >>>>> data->target_count[channel]); > >>>>> break; > >>>>> + case hwmon_fan_enable: > >>>>> + fan_config = data->fan_config[channel]; > >>>>> + if (val == 0) { > >>>>> + fan_config &= ~MAX31790_FAN_CFG_TACH_INPUT_EN; > >>>>> + } else if (val == 1) { > >>>>> + fan_config |= MAX31790_FAN_CFG_TACH_INPUT_EN; > >>>>> + } else { > >>>>> + err = -EINVAL; > >>>>> + break; > >>>>> + } > >>>>> + if (fan_config != data->fan_config[channel]) { > >>>>> + err = i2c_smbus_write_byte_data(client, MAX31790_REG_FAN_CONFIG(channel), > >>>>> + fan_config); > >>>>> + if (!err) > >>>>> + data->fan_config[channel] = fan_config; > >>>>> + } > >>>>> + break; > >>>>> default: > >>>>> err = -EOPNOTSUPP; > >>>>> break; > >>>>> @@ -270,6 +296,10 @@ static umode_t max31790_fan_is_visible(const void *_data, u32 attr, int channel) > >>>>> !(fan_config & MAX31790_FAN_CFG_TACH_INPUT)) > >>>>> return 0644; > >>>>> return 0; > >>>>> + case hwmon_fan_enable: > >>>>> + if (channel < NR_CHANNEL) > >>>>> + return 0644; > >>>>> + return 0; > >>>>> default: > >>>>> return 0; > >>>>> } > >>>>> @@ -423,12 +453,12 @@ 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_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_INPUT | HWMON_F_FAULT, > >>>>> HWMON_F_INPUT | HWMON_F_FAULT, > >>>>> -- > >>>>> 2.37.2.672.g94769d06f0-goog > >>>>> >
diff --git a/Documentation/hwmon/max31790.rst b/Documentation/hwmon/max31790.rst index 7b097c3b9b90..33c5c7330efc 100644 --- a/Documentation/hwmon/max31790.rst +++ b/Documentation/hwmon/max31790.rst @@ -38,6 +38,7 @@ Sysfs entries 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 +fan[1-6]_enable RW enable or disable the tachometer input pwm[1-6]_enable RW regulator mode, 0=disabled (duty cycle=0%), 1=manual mode, 2=rpm mode pwm[1-6] RW read: current pwm duty cycle, write: target pwm duty cycle (0-255) diff --git a/drivers/hwmon/max31790.c b/drivers/hwmon/max31790.c index 7e9362f6dc29..3ae02be4b41e 100644 --- a/drivers/hwmon/max31790.c +++ b/drivers/hwmon/max31790.c @@ -118,6 +118,12 @@ static struct max31790_data *max31790_update_device(struct device *dev) goto abort; data->target_count[i] = rv; } + + rv = i2c_smbus_read_byte_data(client, + MAX31790_REG_FAN_CONFIG(i)); + if (rv < 0) + goto abort; + data->fan_config[i] = rv; } data->last_updated = jiffies; @@ -202,6 +208,9 @@ static int max31790_read_fan(struct device *dev, u32 attr, int channel, } mutex_unlock(&data->update_lock); return 0; + case hwmon_fan_enable: + *val = !!(data->fan_config[channel] & MAX31790_FAN_CFG_TACH_INPUT_EN); + return 0; default: return -EOPNOTSUPP; } @@ -214,7 +223,7 @@ 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, fan_config; int sr; mutex_lock(&data->update_lock); @@ -243,6 +252,23 @@ static int max31790_write_fan(struct device *dev, u32 attr, int channel, MAX31790_REG_TARGET_COUNT(channel), data->target_count[channel]); break; + case hwmon_fan_enable: + fan_config = data->fan_config[channel]; + if (val == 0) { + fan_config &= ~MAX31790_FAN_CFG_TACH_INPUT_EN; + } else if (val == 1) { + fan_config |= MAX31790_FAN_CFG_TACH_INPUT_EN; + } else { + err = -EINVAL; + break; + } + if (fan_config != data->fan_config[channel]) { + err = i2c_smbus_write_byte_data(client, MAX31790_REG_FAN_CONFIG(channel), + fan_config); + if (!err) + data->fan_config[channel] = fan_config; + } + break; default: err = -EOPNOTSUPP; break; @@ -270,6 +296,10 @@ static umode_t max31790_fan_is_visible(const void *_data, u32 attr, int channel) !(fan_config & MAX31790_FAN_CFG_TACH_INPUT)) return 0644; return 0; + case hwmon_fan_enable: + if (channel < NR_CHANNEL) + return 0644; + return 0; default: return 0; } @@ -423,12 +453,12 @@ 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_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_INPUT | HWMON_F_FAULT, HWMON_F_INPUT | HWMON_F_FAULT,
The MAX31790 has a tach input enable bit in each fan's configuration register. This is only enabled by the driver if RPM mode is selected, but the driver doesn't provide a way to independently enable tachometer input regardless of the regulator mode. By adding the fanN_enable sysfs files, we can decouple the tach input from the regulator mode. Also update the documentation. Signed-off-by: Justin Ledford <justinledford@google.com> --- Documentation/hwmon/max31790.rst | 1 + drivers/hwmon/max31790.c | 44 +++++++++++++++++++++++++++----- 2 files changed, 38 insertions(+), 7 deletions(-)