* [PATCH] hwmon: (adt7470) Add support for PWM inversion @ 2021-08-16 20:35 Robert Hancock 2021-08-18 3:26 ` Guenter Roeck 0 siblings, 1 reply; 4+ messages in thread From: Robert Hancock @ 2021-08-16 20:35 UTC (permalink / raw) To: jdelvare, linux; +Cc: linux-hwmon, Robert Hancock Add sysfs attributes to control whether the PWM output signal from the device is inverted (i.e. active-low rather than active-high). Signed-off-by: Robert Hancock <robert.hancock@calian.com> --- Documentation/hwmon/adt7470.rst | 5 ++- drivers/hwmon/adt7470.c | 72 ++++++++++++++++++++++++++++++++- 2 files changed, 75 insertions(+), 2 deletions(-) diff --git a/Documentation/hwmon/adt7470.rst b/Documentation/hwmon/adt7470.rst index d225f816e992..8293f93d6cd5 100644 --- a/Documentation/hwmon/adt7470.rst +++ b/Documentation/hwmon/adt7470.rst @@ -69,7 +69,10 @@ from 0 (off) to 255 (full speed). Fan speed will be set to maximum when the temperature sensor associated with the PWM control exceeds pwm#_auto_point2_temp. -The driver also allows control of the PWM frequency: +The driver also allows control of the PWM inversion and frequency: + +* pwm#_invert: Controls whether the PWM output signal is inverted + (i.e. low when active rather than high). * pwm1_freq diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c index 2e8feacccf84..f0863a6e1560 100644 --- a/drivers/hwmon/adt7470.c +++ b/drivers/hwmon/adt7470.c @@ -61,10 +61,14 @@ static const unsigned short normal_i2c[] = { 0x2C, 0x2E, 0x2F, I2C_CLIENT_END }; #define ADT7470_REG_FAN_MAX_MAX_ADDR 0x67 #define ADT7470_REG_PWM_CFG_BASE_ADDR 0x68 #define ADT7470_REG_PWM12_CFG 0x68 +#define ADT7470_PWM2_INVERT_MASK 0x10 +#define ADT7470_PWM1_INVERT_MASK 0x20 #define ADT7470_PWM2_AUTO_MASK 0x40 #define ADT7470_PWM1_AUTO_MASK 0x80 #define ADT7470_PWM_AUTO_MASK 0xC0 #define ADT7470_REG_PWM34_CFG 0x69 +#define ADT7470_PWM4_INVERT_MASK 0x10 +#define ADT7470_PWM3_INVERT_MASK 0x20 #define ADT7470_PWM3_AUTO_MASK 0x40 #define ADT7470_PWM4_AUTO_MASK 0x80 #define ADT7470_REG_PWM_MIN_BASE_ADDR 0x6A @@ -162,6 +166,7 @@ struct adt7470_data { u8 pwm_min[ADT7470_PWM_COUNT]; s8 pwm_tmin[ADT7470_PWM_COUNT]; u8 pwm_auto_temp[ADT7470_PWM_COUNT]; + u8 pwm_invert[ADT7470_PWM_COUNT]; struct task_struct *auto_update; unsigned int auto_update_interval; @@ -300,11 +305,22 @@ static int adt7470_update_sensors(struct adt7470_data *data) reg_mask = ADT7470_PWM1_AUTO_MASK; reg = ADT7470_REG_PWM_CFG(i); - if (i2c_smbus_read_byte_data(client, reg) & reg_mask) + cfg = i2c_smbus_read_byte_data(client, reg); + if (cfg & reg_mask) data->pwm_automatic[i] = 1; else data->pwm_automatic[i] = 0; + if (i % 2) + reg_mask = ADT7470_PWM2_INVERT_MASK; + else + reg_mask = ADT7470_PWM1_INVERT_MASK; + + if (cfg & reg_mask) + data->pwm_invert[i] = 1; + else + data->pwm_invert[i] = 0; + reg = ADT7470_REG_PWM_AUTO_TEMP(i); cfg = i2c_smbus_read_byte_data(client, reg); if (!(i % 2)) @@ -935,6 +951,51 @@ static ssize_t pwm_tmin_store(struct device *dev, return count; } +static ssize_t pwm_invert_show(struct device *dev, + struct device_attribute *devattr, char *buf) +{ + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); + struct adt7470_data *data = adt7470_update_device(dev); + + return sprintf(buf, "%d\n", (int)data->pwm_invert[attr->index]); +} + +static ssize_t pwm_invert_store(struct device *dev, + struct device_attribute *devattr, + const char *buf, size_t count) +{ + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); + struct adt7470_data *data = dev_get_drvdata(dev); + struct i2c_client *client = data->client; + int pwm_invert_reg = ADT7470_REG_PWM_CFG(attr->index); + int pwm_invert_reg_mask; + long temp; + u8 reg; + + if (kstrtol(buf, 10, &temp)) + return -EINVAL; + + if (attr->index % 2) + pwm_invert_reg_mask = ADT7470_PWM2_INVERT_MASK; + else + pwm_invert_reg_mask = ADT7470_PWM1_INVERT_MASK; + + if (temp != 1 && temp != 0) + return -EINVAL; + + mutex_lock(&data->lock); + data->pwm_invert[attr->index] = temp; + reg = i2c_smbus_read_byte_data(client, pwm_invert_reg); + if (temp) + reg |= pwm_invert_reg_mask; + else + reg &= ~pwm_invert_reg_mask; + i2c_smbus_write_byte_data(client, pwm_invert_reg, reg); + mutex_unlock(&data->lock); + + return count; +} + static ssize_t pwm_auto_show(struct device *dev, struct device_attribute *devattr, char *buf) { @@ -1135,6 +1196,11 @@ static SENSOR_DEVICE_ATTR_RW(pwm4, pwm, 3); static DEVICE_ATTR_RW(pwm1_freq); +static SENSOR_DEVICE_ATTR_RW(pwm1_invert, pwm_invert, 0); +static SENSOR_DEVICE_ATTR_RW(pwm2_invert, pwm_invert, 1); +static SENSOR_DEVICE_ATTR_RW(pwm3_invert, pwm_invert, 2); +static SENSOR_DEVICE_ATTR_RW(pwm4_invert, pwm_invert, 3); + static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point1_pwm, pwm_min, 0); static SENSOR_DEVICE_ATTR_RW(pwm2_auto_point1_pwm, pwm_min, 1); static SENSOR_DEVICE_ATTR_RW(pwm3_auto_point1_pwm, pwm_min, 2); @@ -1231,6 +1297,10 @@ static struct attribute *adt7470_attrs[] = { &sensor_dev_attr_pwm2.dev_attr.attr, &sensor_dev_attr_pwm3.dev_attr.attr, &sensor_dev_attr_pwm4.dev_attr.attr, + &sensor_dev_attr_pwm1_invert.dev_attr.attr, + &sensor_dev_attr_pwm2_invert.dev_attr.attr, + &sensor_dev_attr_pwm3_invert.dev_attr.attr, + &sensor_dev_attr_pwm4_invert.dev_attr.attr, &sensor_dev_attr_pwm1_auto_point1_pwm.dev_attr.attr, &sensor_dev_attr_pwm2_auto_point1_pwm.dev_attr.attr, &sensor_dev_attr_pwm3_auto_point1_pwm.dev_attr.attr, -- 2.27.0 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] hwmon: (adt7470) Add support for PWM inversion 2021-08-16 20:35 [PATCH] hwmon: (adt7470) Add support for PWM inversion Robert Hancock @ 2021-08-18 3:26 ` Guenter Roeck 2021-08-18 16:34 ` Robert Hancock 0 siblings, 1 reply; 4+ messages in thread From: Guenter Roeck @ 2021-08-18 3:26 UTC (permalink / raw) To: Robert Hancock, jdelvare; +Cc: linux-hwmon On 8/16/21 1:35 PM, Robert Hancock wrote: > Add sysfs attributes to control whether the PWM output signal from the > device is inverted (i.e. active-low rather than active-high). > We would normally call that "polarity". Anyway, what is the use case ? The pwm polarity is not a fan property but a system property. While many of the hwmon sysfs attributes describe system properties, that is for the most part a leftover from times when PCs did not provide the necessary information, and it was necessary to configure hwmon devices from userspace. Nowadays, system properties are typically provided with devicetree properties or with the ACPI equivalent. A good example is the driver for adt7575. Would that solve your problem ? Thanks, Guenter > Signed-off-by: Robert Hancock <robert.hancock@calian.com> > --- > Documentation/hwmon/adt7470.rst | 5 ++- > drivers/hwmon/adt7470.c | 72 ++++++++++++++++++++++++++++++++- > 2 files changed, 75 insertions(+), 2 deletions(-) > > diff --git a/Documentation/hwmon/adt7470.rst b/Documentation/hwmon/adt7470.rst > index d225f816e992..8293f93d6cd5 100644 > --- a/Documentation/hwmon/adt7470.rst > +++ b/Documentation/hwmon/adt7470.rst > @@ -69,7 +69,10 @@ from 0 (off) to 255 (full speed). Fan speed will be set to maximum when the > temperature sensor associated with the PWM control exceeds > pwm#_auto_point2_temp. > > -The driver also allows control of the PWM frequency: > +The driver also allows control of the PWM inversion and frequency: > + > +* pwm#_invert: Controls whether the PWM output signal is inverted > + (i.e. low when active rather than high). > > * pwm1_freq > > diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c > index 2e8feacccf84..f0863a6e1560 100644 > --- a/drivers/hwmon/adt7470.c > +++ b/drivers/hwmon/adt7470.c > @@ -61,10 +61,14 @@ static const unsigned short normal_i2c[] = { 0x2C, 0x2E, 0x2F, I2C_CLIENT_END }; > #define ADT7470_REG_FAN_MAX_MAX_ADDR 0x67 > #define ADT7470_REG_PWM_CFG_BASE_ADDR 0x68 > #define ADT7470_REG_PWM12_CFG 0x68 > +#define ADT7470_PWM2_INVERT_MASK 0x10 > +#define ADT7470_PWM1_INVERT_MASK 0x20 > #define ADT7470_PWM2_AUTO_MASK 0x40 > #define ADT7470_PWM1_AUTO_MASK 0x80 > #define ADT7470_PWM_AUTO_MASK 0xC0 > #define ADT7470_REG_PWM34_CFG 0x69 > +#define ADT7470_PWM4_INVERT_MASK 0x10 > +#define ADT7470_PWM3_INVERT_MASK 0x20 > #define ADT7470_PWM3_AUTO_MASK 0x40 > #define ADT7470_PWM4_AUTO_MASK 0x80 > #define ADT7470_REG_PWM_MIN_BASE_ADDR 0x6A > @@ -162,6 +166,7 @@ struct adt7470_data { > u8 pwm_min[ADT7470_PWM_COUNT]; > s8 pwm_tmin[ADT7470_PWM_COUNT]; > u8 pwm_auto_temp[ADT7470_PWM_COUNT]; > + u8 pwm_invert[ADT7470_PWM_COUNT]; > > struct task_struct *auto_update; > unsigned int auto_update_interval; > @@ -300,11 +305,22 @@ static int adt7470_update_sensors(struct adt7470_data *data) > reg_mask = ADT7470_PWM1_AUTO_MASK; > > reg = ADT7470_REG_PWM_CFG(i); > - if (i2c_smbus_read_byte_data(client, reg) & reg_mask) > + cfg = i2c_smbus_read_byte_data(client, reg); > + if (cfg & reg_mask) > data->pwm_automatic[i] = 1; > else > data->pwm_automatic[i] = 0; > > + if (i % 2) > + reg_mask = ADT7470_PWM2_INVERT_MASK; > + else > + reg_mask = ADT7470_PWM1_INVERT_MASK; > + > + if (cfg & reg_mask) > + data->pwm_invert[i] = 1; > + else > + data->pwm_invert[i] = 0; > + > reg = ADT7470_REG_PWM_AUTO_TEMP(i); > cfg = i2c_smbus_read_byte_data(client, reg); > if (!(i % 2)) > @@ -935,6 +951,51 @@ static ssize_t pwm_tmin_store(struct device *dev, > return count; > } > > +static ssize_t pwm_invert_show(struct device *dev, > + struct device_attribute *devattr, char *buf) > +{ > + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); > + struct adt7470_data *data = adt7470_update_device(dev); > + > + return sprintf(buf, "%d\n", (int)data->pwm_invert[attr->index]); > +} > + > +static ssize_t pwm_invert_store(struct device *dev, > + struct device_attribute *devattr, > + const char *buf, size_t count) > +{ > + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); > + struct adt7470_data *data = dev_get_drvdata(dev); > + struct i2c_client *client = data->client; > + int pwm_invert_reg = ADT7470_REG_PWM_CFG(attr->index); > + int pwm_invert_reg_mask; > + long temp; > + u8 reg; > + > + if (kstrtol(buf, 10, &temp)) > + return -EINVAL; > + > + if (attr->index % 2) > + pwm_invert_reg_mask = ADT7470_PWM2_INVERT_MASK; > + else > + pwm_invert_reg_mask = ADT7470_PWM1_INVERT_MASK; > + > + if (temp != 1 && temp != 0) > + return -EINVAL; > + > + mutex_lock(&data->lock); > + data->pwm_invert[attr->index] = temp; > + reg = i2c_smbus_read_byte_data(client, pwm_invert_reg); > + if (temp) > + reg |= pwm_invert_reg_mask; > + else > + reg &= ~pwm_invert_reg_mask; > + i2c_smbus_write_byte_data(client, pwm_invert_reg, reg); > + mutex_unlock(&data->lock); > + > + return count; > +} > + > static ssize_t pwm_auto_show(struct device *dev, > struct device_attribute *devattr, char *buf) > { > @@ -1135,6 +1196,11 @@ static SENSOR_DEVICE_ATTR_RW(pwm4, pwm, 3); > > static DEVICE_ATTR_RW(pwm1_freq); > > +static SENSOR_DEVICE_ATTR_RW(pwm1_invert, pwm_invert, 0); > +static SENSOR_DEVICE_ATTR_RW(pwm2_invert, pwm_invert, 1); > +static SENSOR_DEVICE_ATTR_RW(pwm3_invert, pwm_invert, 2); > +static SENSOR_DEVICE_ATTR_RW(pwm4_invert, pwm_invert, 3); > + > static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point1_pwm, pwm_min, 0); > static SENSOR_DEVICE_ATTR_RW(pwm2_auto_point1_pwm, pwm_min, 1); > static SENSOR_DEVICE_ATTR_RW(pwm3_auto_point1_pwm, pwm_min, 2); > @@ -1231,6 +1297,10 @@ static struct attribute *adt7470_attrs[] = { > &sensor_dev_attr_pwm2.dev_attr.attr, > &sensor_dev_attr_pwm3.dev_attr.attr, > &sensor_dev_attr_pwm4.dev_attr.attr, > + &sensor_dev_attr_pwm1_invert.dev_attr.attr, > + &sensor_dev_attr_pwm2_invert.dev_attr.attr, > + &sensor_dev_attr_pwm3_invert.dev_attr.attr, > + &sensor_dev_attr_pwm4_invert.dev_attr.attr, > &sensor_dev_attr_pwm1_auto_point1_pwm.dev_attr.attr, > &sensor_dev_attr_pwm2_auto_point1_pwm.dev_attr.attr, > &sensor_dev_attr_pwm3_auto_point1_pwm.dev_attr.attr, > ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] hwmon: (adt7470) Add support for PWM inversion 2021-08-18 3:26 ` Guenter Roeck @ 2021-08-18 16:34 ` Robert Hancock 2021-08-18 17:53 ` Guenter Roeck 0 siblings, 1 reply; 4+ messages in thread From: Robert Hancock @ 2021-08-18 16:34 UTC (permalink / raw) To: linux, jdelvare; +Cc: linux-hwmon On Tue, 2021-08-17 at 20:26 -0700, Guenter Roeck wrote: > On 8/16/21 1:35 PM, Robert Hancock wrote: > > Add sysfs attributes to control whether the PWM output signal from the > > device is inverted (i.e. active-low rather than active-high). > > > > We would normally call that "polarity". > > Anyway, what is the use case ? The pwm polarity is not a fan property but > a system property. While many of the hwmon sysfs attributes describe system > properties, that is for the most part a leftover from times when PCs > did not provide the necessary information, and it was necessary to > configure hwmon devices from userspace. > > Nowadays, system properties are typically provided with devicetree > properties or with the ACPI equivalent. A good example is the driver > for adt7575. Would that solve your problem ? In our case the board we are using with this device has a transistor between the ADT7470 PWM output and the fan PWM input, which results in the signal being inverted. So this is effectively a hardware property, which could belong in the device tree. The PWM frequency is another property we are setting that could belong there as well. Right now the ADT7470 driver doesn't do anything with the device tree at all, the entry in the DT for the device is only useful for probing. I suppose it could get some code to initialize the default settings for some of these values from the device tree if they are present and still provide the sysfs interface for backward compatibility? You mentioned adt7575, I assume you meant adt7475 - it has an adi,pwm-active- state value that can be defined to set the PWM as active low or active high for each output. We could do something similar here. Not sure if anything else is setting PWM frequencies in the device tree right now? I see some in-tree DTSes which set a maxim,fan-pwm-freq property but I don't see any driver in tree that reads that property. > > Thanks, > Guenter > > > Signed-off-by: Robert Hancock <robert.hancock@calian.com> > > --- > > Documentation/hwmon/adt7470.rst | 5 ++- > > drivers/hwmon/adt7470.c | 72 ++++++++++++++++++++++++++++++++- > > 2 files changed, 75 insertions(+), 2 deletions(-) > > > > diff --git a/Documentation/hwmon/adt7470.rst > > b/Documentation/hwmon/adt7470.rst > > index d225f816e992..8293f93d6cd5 100644 > > --- a/Documentation/hwmon/adt7470.rst > > +++ b/Documentation/hwmon/adt7470.rst > > @@ -69,7 +69,10 @@ from 0 (off) to 255 (full speed). Fan speed will be set > > to maximum when the > > temperature sensor associated with the PWM control exceeds > > pwm#_auto_point2_temp. > > > > -The driver also allows control of the PWM frequency: > > +The driver also allows control of the PWM inversion and frequency: > > + > > +* pwm#_invert: Controls whether the PWM output signal is inverted > > + (i.e. low when active rather than high). > > > > * pwm1_freq > > > > diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c > > index 2e8feacccf84..f0863a6e1560 100644 > > --- a/drivers/hwmon/adt7470.c > > +++ b/drivers/hwmon/adt7470.c > > @@ -61,10 +61,14 @@ static const unsigned short normal_i2c[] = { 0x2C, > > 0x2E, 0x2F, I2C_CLIENT_END }; > > #define ADT7470_REG_FAN_MAX_MAX_ADDR 0x67 > > #define ADT7470_REG_PWM_CFG_BASE_ADDR 0x68 > > #define ADT7470_REG_PWM12_CFG 0x68 > > +#define ADT7470_PWM2_INVERT_MASK 0x10 > > +#define ADT7470_PWM1_INVERT_MASK 0x20 > > #define ADT7470_PWM2_AUTO_MASK 0x40 > > #define ADT7470_PWM1_AUTO_MASK 0x80 > > #define ADT7470_PWM_AUTO_MASK 0xC0 > > #define ADT7470_REG_PWM34_CFG 0x69 > > +#define ADT7470_PWM4_INVERT_MASK 0x10 > > +#define ADT7470_PWM3_INVERT_MASK 0x20 > > #define ADT7470_PWM3_AUTO_MASK 0x40 > > #define ADT7470_PWM4_AUTO_MASK 0x80 > > #define ADT7470_REG_PWM_MIN_BASE_ADDR 0x6A > > @@ -162,6 +166,7 @@ struct adt7470_data { > > u8 pwm_min[ADT7470_PWM_COUNT]; > > s8 pwm_tmin[ADT7470_PWM_COUNT]; > > u8 pwm_auto_temp[ADT7470_PWM_COUNT]; > > + u8 pwm_invert[ADT7470_PWM_COUNT]; > > > > struct task_struct *auto_update; > > unsigned int auto_update_interval; > > @@ -300,11 +305,22 @@ static int adt7470_update_sensors(struct adt7470_data > > *data) > > reg_mask = ADT7470_PWM1_AUTO_MASK; > > > > reg = ADT7470_REG_PWM_CFG(i); > > - if (i2c_smbus_read_byte_data(client, reg) & reg_mask) > > + cfg = i2c_smbus_read_byte_data(client, reg); > > + if (cfg & reg_mask) > > data->pwm_automatic[i] = 1; > > else > > data->pwm_automatic[i] = 0; > > > > + if (i % 2) > > + reg_mask = ADT7470_PWM2_INVERT_MASK; > > + else > > + reg_mask = ADT7470_PWM1_INVERT_MASK; > > + > > + if (cfg & reg_mask) > > + data->pwm_invert[i] = 1; > > + else > > + data->pwm_invert[i] = 0; > > + > > reg = ADT7470_REG_PWM_AUTO_TEMP(i); > > cfg = i2c_smbus_read_byte_data(client, reg); > > if (!(i % 2)) > > @@ -935,6 +951,51 @@ static ssize_t pwm_tmin_store(struct device *dev, > > return count; > > } > > > > +static ssize_t pwm_invert_show(struct device *dev, > > + struct device_attribute *devattr, char *buf) > > +{ > > + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); > > + struct adt7470_data *data = adt7470_update_device(dev); > > + > > + return sprintf(buf, "%d\n", (int)data->pwm_invert[attr->index]); > > +} > > + > > +static ssize_t pwm_invert_store(struct device *dev, > > + struct device_attribute *devattr, > > + const char *buf, size_t count) > > +{ > > + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); > > + struct adt7470_data *data = dev_get_drvdata(dev); > > + struct i2c_client *client = data->client; > > + int pwm_invert_reg = ADT7470_REG_PWM_CFG(attr->index); > > + int pwm_invert_reg_mask; > > + long temp; > > + u8 reg; > > + > > + if (kstrtol(buf, 10, &temp)) > > + return -EINVAL; > > + > > + if (attr->index % 2) > > + pwm_invert_reg_mask = ADT7470_PWM2_INVERT_MASK; > > + else > > + pwm_invert_reg_mask = ADT7470_PWM1_INVERT_MASK; > > + > > + if (temp != 1 && temp != 0) > > + return -EINVAL; > > + > > + mutex_lock(&data->lock); > > + data->pwm_invert[attr->index] = temp; > > + reg = i2c_smbus_read_byte_data(client, pwm_invert_reg); > > + if (temp) > > + reg |= pwm_invert_reg_mask; > > + else > > + reg &= ~pwm_invert_reg_mask; > > + i2c_smbus_write_byte_data(client, pwm_invert_reg, reg); > > + mutex_unlock(&data->lock); > > + > > + return count; > > +} > > + > > static ssize_t pwm_auto_show(struct device *dev, > > struct device_attribute *devattr, char *buf) > > { > > @@ -1135,6 +1196,11 @@ static SENSOR_DEVICE_ATTR_RW(pwm4, pwm, 3); > > > > static DEVICE_ATTR_RW(pwm1_freq); > > > > +static SENSOR_DEVICE_ATTR_RW(pwm1_invert, pwm_invert, 0); > > +static SENSOR_DEVICE_ATTR_RW(pwm2_invert, pwm_invert, 1); > > +static SENSOR_DEVICE_ATTR_RW(pwm3_invert, pwm_invert, 2); > > +static SENSOR_DEVICE_ATTR_RW(pwm4_invert, pwm_invert, 3); > > + > > static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point1_pwm, pwm_min, 0); > > static SENSOR_DEVICE_ATTR_RW(pwm2_auto_point1_pwm, pwm_min, 1); > > static SENSOR_DEVICE_ATTR_RW(pwm3_auto_point1_pwm, pwm_min, 2); > > @@ -1231,6 +1297,10 @@ static struct attribute *adt7470_attrs[] = { > > &sensor_dev_attr_pwm2.dev_attr.attr, > > &sensor_dev_attr_pwm3.dev_attr.attr, > > &sensor_dev_attr_pwm4.dev_attr.attr, > > + &sensor_dev_attr_pwm1_invert.dev_attr.attr, > > + &sensor_dev_attr_pwm2_invert.dev_attr.attr, > > + &sensor_dev_attr_pwm3_invert.dev_attr.attr, > > + &sensor_dev_attr_pwm4_invert.dev_attr.attr, > > &sensor_dev_attr_pwm1_auto_point1_pwm.dev_attr.attr, > > &sensor_dev_attr_pwm2_auto_point1_pwm.dev_attr.attr, > > &sensor_dev_attr_pwm3_auto_point1_pwm.dev_attr.attr, > > -- Robert Hancock Senior Hardware Designer, Calian Advanced Technologies www.calian.com ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] hwmon: (adt7470) Add support for PWM inversion 2021-08-18 16:34 ` Robert Hancock @ 2021-08-18 17:53 ` Guenter Roeck 0 siblings, 0 replies; 4+ messages in thread From: Guenter Roeck @ 2021-08-18 17:53 UTC (permalink / raw) To: Robert Hancock, jdelvare; +Cc: linux-hwmon On 8/18/21 9:34 AM, Robert Hancock wrote: > On Tue, 2021-08-17 at 20:26 -0700, Guenter Roeck wrote: >> On 8/16/21 1:35 PM, Robert Hancock wrote: >>> Add sysfs attributes to control whether the PWM output signal from the >>> device is inverted (i.e. active-low rather than active-high). >>> >> >> We would normally call that "polarity". >> >> Anyway, what is the use case ? The pwm polarity is not a fan property but >> a system property. While many of the hwmon sysfs attributes describe system >> properties, that is for the most part a leftover from times when PCs >> did not provide the necessary information, and it was necessary to >> configure hwmon devices from userspace. >> >> Nowadays, system properties are typically provided with devicetree >> properties or with the ACPI equivalent. A good example is the driver >> for adt7575. Would that solve your problem ? > > In our case the board we are using with this device has a transistor between > the ADT7470 PWM output and the fan PWM input, which results in the signal being > inverted. So this is effectively a hardware property, which could belong in the > device tree. The PWM frequency is another property we are setting that could > belong there as well. > > Right now the ADT7470 driver doesn't do anything with the device tree at all, > the entry in the DT for the device is only useful for probing. I suppose it > could get some code to initialize the default settings for some of these values > from the device tree if they are present and still provide the sysfs interface > for backward compatibility? > Sure, but only for the existing attributes. > You mentioned adt7575, I assume you meant adt7475 - it has an adi,pwm-active- Yes, sorry. Fat finger. > state value that can be defined to set the PWM as active low or active high for > each output. We could do something similar here. Not sure if anything else is > setting PWM frequencies in the device tree right now? I see some in-tree DTSes > which set a maxim,fan-pwm-freq property but I don't see any driver in tree that > reads that property. > The definition was submitted recently but Rob didn't like it because the properties are listed as Maxim-specific but should be generic. At some point someone will have to spend the time and propose a set of standard properties. Anyway, the pwm subsystem uses 'clocks' properties for this purpose. The npcm750-pwm-fan driver uses the same mechanism. Maybe we should use that here. Thanks, Guenter >> >> Thanks, >> Guenter >> >>> Signed-off-by: Robert Hancock <robert.hancock@calian.com> >>> --- >>> Documentation/hwmon/adt7470.rst | 5 ++- >>> drivers/hwmon/adt7470.c | 72 ++++++++++++++++++++++++++++++++- >>> 2 files changed, 75 insertions(+), 2 deletions(-) >>> >>> diff --git a/Documentation/hwmon/adt7470.rst >>> b/Documentation/hwmon/adt7470.rst >>> index d225f816e992..8293f93d6cd5 100644 >>> --- a/Documentation/hwmon/adt7470.rst >>> +++ b/Documentation/hwmon/adt7470.rst >>> @@ -69,7 +69,10 @@ from 0 (off) to 255 (full speed). Fan speed will be set >>> to maximum when the >>> temperature sensor associated with the PWM control exceeds >>> pwm#_auto_point2_temp. >>> >>> -The driver also allows control of the PWM frequency: >>> +The driver also allows control of the PWM inversion and frequency: >>> + >>> +* pwm#_invert: Controls whether the PWM output signal is inverted >>> + (i.e. low when active rather than high). >>> >>> * pwm1_freq >>> >>> diff --git a/drivers/hwmon/adt7470.c b/drivers/hwmon/adt7470.c >>> index 2e8feacccf84..f0863a6e1560 100644 >>> --- a/drivers/hwmon/adt7470.c >>> +++ b/drivers/hwmon/adt7470.c >>> @@ -61,10 +61,14 @@ static const unsigned short normal_i2c[] = { 0x2C, >>> 0x2E, 0x2F, I2C_CLIENT_END }; >>> #define ADT7470_REG_FAN_MAX_MAX_ADDR 0x67 >>> #define ADT7470_REG_PWM_CFG_BASE_ADDR 0x68 >>> #define ADT7470_REG_PWM12_CFG 0x68 >>> +#define ADT7470_PWM2_INVERT_MASK 0x10 >>> +#define ADT7470_PWM1_INVERT_MASK 0x20 >>> #define ADT7470_PWM2_AUTO_MASK 0x40 >>> #define ADT7470_PWM1_AUTO_MASK 0x80 >>> #define ADT7470_PWM_AUTO_MASK 0xC0 >>> #define ADT7470_REG_PWM34_CFG 0x69 >>> +#define ADT7470_PWM4_INVERT_MASK 0x10 >>> +#define ADT7470_PWM3_INVERT_MASK 0x20 >>> #define ADT7470_PWM3_AUTO_MASK 0x40 >>> #define ADT7470_PWM4_AUTO_MASK 0x80 >>> #define ADT7470_REG_PWM_MIN_BASE_ADDR 0x6A >>> @@ -162,6 +166,7 @@ struct adt7470_data { >>> u8 pwm_min[ADT7470_PWM_COUNT]; >>> s8 pwm_tmin[ADT7470_PWM_COUNT]; >>> u8 pwm_auto_temp[ADT7470_PWM_COUNT]; >>> + u8 pwm_invert[ADT7470_PWM_COUNT]; >>> >>> struct task_struct *auto_update; >>> unsigned int auto_update_interval; >>> @@ -300,11 +305,22 @@ static int adt7470_update_sensors(struct adt7470_data >>> *data) >>> reg_mask = ADT7470_PWM1_AUTO_MASK; >>> >>> reg = ADT7470_REG_PWM_CFG(i); >>> - if (i2c_smbus_read_byte_data(client, reg) & reg_mask) >>> + cfg = i2c_smbus_read_byte_data(client, reg); >>> + if (cfg & reg_mask) >>> data->pwm_automatic[i] = 1; >>> else >>> data->pwm_automatic[i] = 0; >>> >>> + if (i % 2) >>> + reg_mask = ADT7470_PWM2_INVERT_MASK; >>> + else >>> + reg_mask = ADT7470_PWM1_INVERT_MASK; >>> + >>> + if (cfg & reg_mask) >>> + data->pwm_invert[i] = 1; >>> + else >>> + data->pwm_invert[i] = 0; >>> + >>> reg = ADT7470_REG_PWM_AUTO_TEMP(i); >>> cfg = i2c_smbus_read_byte_data(client, reg); >>> if (!(i % 2)) >>> @@ -935,6 +951,51 @@ static ssize_t pwm_tmin_store(struct device *dev, >>> return count; >>> } >>> >>> +static ssize_t pwm_invert_show(struct device *dev, >>> + struct device_attribute *devattr, char *buf) >>> +{ >>> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); >>> + struct adt7470_data *data = adt7470_update_device(dev); >>> + >>> + return sprintf(buf, "%d\n", (int)data->pwm_invert[attr->index]); >>> +} >>> + >>> +static ssize_t pwm_invert_store(struct device *dev, >>> + struct device_attribute *devattr, >>> + const char *buf, size_t count) >>> +{ >>> + struct sensor_device_attribute *attr = to_sensor_dev_attr(devattr); >>> + struct adt7470_data *data = dev_get_drvdata(dev); >>> + struct i2c_client *client = data->client; >>> + int pwm_invert_reg = ADT7470_REG_PWM_CFG(attr->index); >>> + int pwm_invert_reg_mask; >>> + long temp; >>> + u8 reg; >>> + >>> + if (kstrtol(buf, 10, &temp)) >>> + return -EINVAL; >>> + >>> + if (attr->index % 2) >>> + pwm_invert_reg_mask = ADT7470_PWM2_INVERT_MASK; >>> + else >>> + pwm_invert_reg_mask = ADT7470_PWM1_INVERT_MASK; >>> + >>> + if (temp != 1 && temp != 0) >>> + return -EINVAL; >>> + >>> + mutex_lock(&data->lock); >>> + data->pwm_invert[attr->index] = temp; >>> + reg = i2c_smbus_read_byte_data(client, pwm_invert_reg); >>> + if (temp) >>> + reg |= pwm_invert_reg_mask; >>> + else >>> + reg &= ~pwm_invert_reg_mask; >>> + i2c_smbus_write_byte_data(client, pwm_invert_reg, reg); >>> + mutex_unlock(&data->lock); >>> + >>> + return count; >>> +} >>> + >>> static ssize_t pwm_auto_show(struct device *dev, >>> struct device_attribute *devattr, char *buf) >>> { >>> @@ -1135,6 +1196,11 @@ static SENSOR_DEVICE_ATTR_RW(pwm4, pwm, 3); >>> >>> static DEVICE_ATTR_RW(pwm1_freq); >>> >>> +static SENSOR_DEVICE_ATTR_RW(pwm1_invert, pwm_invert, 0); >>> +static SENSOR_DEVICE_ATTR_RW(pwm2_invert, pwm_invert, 1); >>> +static SENSOR_DEVICE_ATTR_RW(pwm3_invert, pwm_invert, 2); >>> +static SENSOR_DEVICE_ATTR_RW(pwm4_invert, pwm_invert, 3); >>> + >>> static SENSOR_DEVICE_ATTR_RW(pwm1_auto_point1_pwm, pwm_min, 0); >>> static SENSOR_DEVICE_ATTR_RW(pwm2_auto_point1_pwm, pwm_min, 1); >>> static SENSOR_DEVICE_ATTR_RW(pwm3_auto_point1_pwm, pwm_min, 2); >>> @@ -1231,6 +1297,10 @@ static struct attribute *adt7470_attrs[] = { >>> &sensor_dev_attr_pwm2.dev_attr.attr, >>> &sensor_dev_attr_pwm3.dev_attr.attr, >>> &sensor_dev_attr_pwm4.dev_attr.attr, >>> + &sensor_dev_attr_pwm1_invert.dev_attr.attr, >>> + &sensor_dev_attr_pwm2_invert.dev_attr.attr, >>> + &sensor_dev_attr_pwm3_invert.dev_attr.attr, >>> + &sensor_dev_attr_pwm4_invert.dev_attr.attr, >>> &sensor_dev_attr_pwm1_auto_point1_pwm.dev_attr.attr, >>> &sensor_dev_attr_pwm2_auto_point1_pwm.dev_attr.attr, >>> &sensor_dev_attr_pwm3_auto_point1_pwm.dev_attr.attr, >>> ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-08-18 17:53 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-08-16 20:35 [PATCH] hwmon: (adt7470) Add support for PWM inversion Robert Hancock 2021-08-18 3:26 ` Guenter Roeck 2021-08-18 16:34 ` Robert Hancock 2021-08-18 17:53 ` Guenter Roeck
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.