| Message ID | 20191218024238.19836-2-logan.shaw@alliedtelesis.co.nz (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | hwmon: (adt7475) Added attenuator bypass support | expand |
On 12/17/19 6:42 PM, Logan Shaw wrote: > Added support for reading and writing the individual and global voltage > attenuator bypasses. Added loading DTS properties to bypass attenuators on > device probe. > There is no explanation why it would be necessary or desirable (or even make sense) to have a non-standard sysfs attribute for this (or, in other words, why this would need to be runtime configurable). The devicetree property is not documented. NACK for the sysfs attribute unless there is an explanation for the need to configure it dynamically. The devicetree property needs to be documented and approved. The datasheet for ADC7475 does not say anything about the ability to control attenuators other than for vcc in configuration register 4. Bit 4, 6, and 7 are listed as unused/reserved, suggesting that those bits - if at all - are only defined for other chips. Nothing in this patch suggests what those chips are. Attenuation bits need to be validated against the chip type. More comments inline. > Signed-off-by: Logan Shaw <logan.shaw@alliedtelesis.co.nz> > --- > --- > drivers/hwmon/adt7475.c | 163 +++++++++++++++++++++++++++++++++++++++- > 1 file changed, 162 insertions(+), 1 deletion(-) > > diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c > index 6c64d50c9aae..92a4be9e33eb 100644 > --- a/drivers/hwmon/adt7475.c > +++ b/drivers/hwmon/adt7475.c > @@ -19,6 +19,7 @@ > #include <linux/hwmon-vid.h> > #include <linux/err.h> > #include <linux/jiffies.h> > +#include <linux/of.h> > #include <linux/util_macros.h> > > /* Indexes for the sysfs hooks */ > @@ -39,6 +40,7 @@ > > #define ALARM 9 > #define FAULT 10 > +#define ATTENUATE 11 > > /* 7475 Common Registers */ > > @@ -126,9 +128,11 @@ > #define ADT7475_TACH_COUNT 4 > #define ADT7475_PWM_COUNT 3 > > -/* Macro to read the registers */ > +/* Macros to read and write registers */ > > #define adt7475_read(reg) i2c_smbus_read_byte_data(client, (reg)) > +#define adt7475_write(reg, data) i2c_smbus_write_byte_data(client, (reg), \ > + (data)) Those are quite pointless macros. Worse, they use a hidden parameter which should be a no-go to start with. I would accept a patch to remove the first one, but I won't accept a patch adding yet another pointless macro. > > /* Macros to easily index the registers */ > > @@ -534,6 +538,82 @@ static ssize_t temp_store(struct device *dev, struct device_attribute *attr, > return count; > } > > +/** > + * Gets the attenuator bit index for a sattr_index. > + * > + * If there is no attenuator bit index for a given sattr_index then returns > + * a negative error code. > + */ > +static int config4_attenuate_index(char sattr_index) > +{ > + int index = -EBADR; > + > + switch (sattr_index) { > + case 0: > + index = 4; > + break; > + case 1: > + index = 5; > + break; > + case 3: > + index = 6; > + break; > + case 4: > + index = 7; > + break; > + } > + > + return index; > +} This function won't be called for non-supported values of sattr_index. The calling code doesn't even check the return value for errors. This is unacceptable, and can be simplified with an if/else. Besides, the the correct index could be set directly in SENSOR_DEVICE_ATTR_2_RW, making the function quite pointless. > + > +/** > + * Gets the bypass attenuator bit for a voltage input and stores it in the char > + * array buf. > + */ > +static ssize_t bypass_attenuator_show(struct device *dev, > + struct device_attribute *attr, char *buf) > +{ > + struct adt7475_data *data = adt7475_update_device(dev); > + struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr); > + u8 attn_bypassed = !!(data->config4 & > + (1 << config4_attenuate_index(sattr->index))); > + if (IS_ERR(data)) > + return PTR_ERR(data); > + > + return sprintf(buf, "%d\n", attn_bypassed); > +} > + > +/** > + * Sets the bypass attenuator bit for a given voltage input. > + */ > +static ssize_t bypass_attenuator_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr); > + struct i2c_client *client = to_i2c_client(dev); > + struct adt7475_data *data = i2c_get_clientdata(client); > + long val; > + > + if (kstrtol(buf, 2, &val)) > + return -EINVAL; > + > + mutex_lock(&data->lock); > + data->config4 = adt7475_read(REG_CONFIG4); > + if (data->config4 < 0) > + return data->config4; > + > + if (val == 0) > + data->config4 &= ~(1 << config4_attenuate_index(sattr->index)); > + else > + data->config4 |= (1 << config4_attenuate_index(sattr->index)); > + > + adt7475_write(REG_CONFIG4, data->config4); > + mutex_unlock(&data->lock); > + > + return count; > +} > + > /* Assuming CONFIG6[SLOW] is 0 */ > static const int ad7475_st_map[] = { > 37500, 18800, 12500, 7500, 4700, 3100, 1600, 800, > @@ -1080,10 +1160,14 @@ static SENSOR_DEVICE_ATTR_2_RO(in0_input, voltage, INPUT, 0); > static SENSOR_DEVICE_ATTR_2_RW(in0_max, voltage, MAX, 0); > static SENSOR_DEVICE_ATTR_2_RW(in0_min, voltage, MIN, 0); > static SENSOR_DEVICE_ATTR_2_RO(in0_alarm, voltage, ALARM, 0); > +static SENSOR_DEVICE_ATTR_2_RW(in0_attenuator_bypass, bypass_attenuator, > + ATTENUATE, 0); > static SENSOR_DEVICE_ATTR_2_RO(in1_input, voltage, INPUT, 1); > static SENSOR_DEVICE_ATTR_2_RW(in1_max, voltage, MAX, 1); > static SENSOR_DEVICE_ATTR_2_RW(in1_min, voltage, MIN, 1); > static SENSOR_DEVICE_ATTR_2_RO(in1_alarm, voltage, ALARM, 1); > +static SENSOR_DEVICE_ATTR_2_RW(in1_attenuator_bypass, bypass_attenuator, > + ATTENUATE, 1); > static SENSOR_DEVICE_ATTR_2_RO(in2_input, voltage, INPUT, 2); > static SENSOR_DEVICE_ATTR_2_RW(in2_max, voltage, MAX, 2); > static SENSOR_DEVICE_ATTR_2_RW(in2_min, voltage, MIN, 2); > @@ -1092,10 +1176,14 @@ static SENSOR_DEVICE_ATTR_2_RO(in3_input, voltage, INPUT, 3); > static SENSOR_DEVICE_ATTR_2_RW(in3_max, voltage, MAX, 3); > static SENSOR_DEVICE_ATTR_2_RW(in3_min, voltage, MIN, 3); > static SENSOR_DEVICE_ATTR_2_RO(in3_alarm, voltage, ALARM, 3); > +static SENSOR_DEVICE_ATTR_2_RW(in3_attenuator_bypass, bypass_attenuator, > + ATTENUATE, 3); > static SENSOR_DEVICE_ATTR_2_RO(in4_input, voltage, INPUT, 4); > static SENSOR_DEVICE_ATTR_2_RW(in4_max, voltage, MAX, 4); > static SENSOR_DEVICE_ATTR_2_RW(in4_min, voltage, MIN, 4); > static SENSOR_DEVICE_ATTR_2_RO(in4_alarm, voltage, ALARM, 8); > +static SENSOR_DEVICE_ATTR_2_RW(in4_attenuator_bypass, bypass_attenuator, > + ATTENUATE, 4); > static SENSOR_DEVICE_ATTR_2_RO(in5_input, voltage, INPUT, 5); > static SENSOR_DEVICE_ATTR_2_RW(in5_max, voltage, MAX, 5); > static SENSOR_DEVICE_ATTR_2_RW(in5_min, voltage, MIN, 5); > @@ -1177,6 +1265,7 @@ static struct attribute *adt7475_attrs[] = { > &sensor_dev_attr_in1_max.dev_attr.attr, > &sensor_dev_attr_in1_min.dev_attr.attr, > &sensor_dev_attr_in1_alarm.dev_attr.attr, > + &sensor_dev_attr_in1_attenuator_bypass.dev_attr.attr, > &sensor_dev_attr_in2_input.dev_attr.attr, > &sensor_dev_attr_in2_max.dev_attr.attr, > &sensor_dev_attr_in2_min.dev_attr.attr, > @@ -1263,6 +1352,7 @@ static struct attribute *in0_attrs[] = { > &sensor_dev_attr_in0_max.dev_attr.attr, > &sensor_dev_attr_in0_min.dev_attr.attr, > &sensor_dev_attr_in0_alarm.dev_attr.attr, > + &sensor_dev_attr_in0_attenuator_bypass.dev_attr.attr, > NULL > }; > > @@ -1271,6 +1361,7 @@ static struct attribute *in3_attrs[] = { > &sensor_dev_attr_in3_max.dev_attr.attr, > &sensor_dev_attr_in3_min.dev_attr.attr, > &sensor_dev_attr_in3_alarm.dev_attr.attr, > + &sensor_dev_attr_in3_attenuator_bypass.dev_attr.attr, > NULL > }; > > @@ -1279,6 +1370,7 @@ static struct attribute *in4_attrs[] = { > &sensor_dev_attr_in4_max.dev_attr.attr, > &sensor_dev_attr_in4_min.dev_attr.attr, > &sensor_dev_attr_in4_alarm.dev_attr.attr, > + &sensor_dev_attr_in4_attenuator_bypass.dev_attr.attr, > NULL > }; > > @@ -1457,6 +1549,69 @@ static int adt7475_update_limits(struct i2c_client *client) > return 0; > } > > +/** Is this an API function that would warrant documentation ? > + * Reads individual voltage input bypass attenuator properties from the DTS, > + * and if the property is present the corresponding bit is set in the > + * register. > + * > + * Properties must be in the form of "bypass-attenuator-inx", where x is an > + * integer from the set {0, 1, 3, 4} (can not bypass in2 attenuator). > + * > + * Returns a negative error code if there was an error writing to the register. > + */ > +static int load_individual_bypass_attenuators(const struct i2c_client *client, > + u8 *config4) > +{ > + char buf[32]; > + int attenuate_index, input_index; > + u8 config4_copy = *config4; > + > + for (input_index = 0; input_index < 5; input_index++) { > + attenuate_index = config4_attenuate_index(input_index); > + if (attenuate_index < 0) > + continue; > + > + sprintf(buf, "bypass-attenuator-in%d", input_index); > + if (of_get_property(client->dev.of_node, buf, NULL)) > + config4_copy |= (1 << attenuate_index); > + } > + > + if (adt7475_write(REG_CONFIG4, config4_copy) < 0) > + // Failed to update register Please follow Documentation/hwmon/submitting-patches.rst. > + return -EREMOTEIO; > + > + *config4 = config4_copy; > + > + return 0; > +} > + > +/** > + * Sets the bypass all attenuators bit, if the "bypass-attenuator-all" > + * property exists in the DTS. > + * > + * Returns a negative error code if there was an error writing to the > + * register. > + */ > +static int load_all_bypass_attenuator(const struct i2c_client *client, > + u8 *config2) > +{ > + u8 config2_copy = *config2; > + > + if (!of_get_property(client->dev.of_node, > + "bypass-attenuator-all", NULL)) > + return 0; > + > + config2_copy |= (1 << 5); > + > + if (adt7475_write(REG_CONFIG2, config2_copy) < 0) > + // failed to write to register > + return -EREMOTEIO; > + > + *config2 = config2_copy; > + > + return 0; > +} > + > static int adt7475_probe(struct i2c_client *client, > const struct i2c_device_id *id) > { > @@ -1545,7 +1700,13 @@ static int adt7475_probe(struct i2c_client *client, > } > > /* Voltage attenuators can be bypassed, globally or individually */ > + if (load_individual_bypass_attenuators(client, &(data->config4)) < 0) > + dev_warn(&client->dev, > + "Error setting bypass attenuator bits\n"); > + > config2 = adt7475_read(REG_CONFIG2); > + if (load_all_bypass_attenuator(client, &config2) < 0) > + dev_warn(&client->dev, "Error setting bypass all attenuator\n"); > if (config2 & CONFIG2_ATTN) { > data->bypass_attn = (0x3 << 3) | 0x3; > } else { >
On Tue, 2019-12-17 at 19:17 -0800, Guenter Roeck wrote: > On 12/17/19 6:42 PM, Logan Shaw wrote: > > Added support for reading and writing the individual and global > > voltage > > attenuator bypasses. Added loading DTS properties to bypass > > attenuators on > > device probe. > > > > There is no explanation why it would be necessary or desirable (or > even make sense) > to have a non-standard sysfs attribute for this (or, in other words, > why this would > need to be runtime configurable). The devicetree property is not > documented. > > NACK for the sysfs attribute unless there is an explanation for the > need > to configure it dynamically. The devicetree property needs to be > documented > and approved. You are correct, it being runtime configurable has little use that I can think of. If anything, it was for completeness and should this patch be worthwhile continuing with I will remove it (and document the devicetree properties). > > The datasheet for ADC7475 does not say anything about the ability to > control > attenuators other than for vcc in configuration register 4. Bit 4, 6, > and 7 > are listed as unused/reserved, suggesting that those bits - if at all > - are > only defined for other chips. Nothing in this patch suggests what > those chips > are. Attenuation bits need to be validated against the chip type. You are right, I missed including important details. The ADT7476 and ADT7490 datasheets specify "Bits [7:4] of Configuration Register 4 (0x7D) can be used to bypass individual voltage channel attenuators". My thought process was it would be up to the person configuring the devicetree to only add the attributes where appropiate (for example, not for a ADT7475 chip). I can see this is dangerious. Instead would it be acceptable to add a check to the load_individual_bypass_attenuators and load_all_bypass_attenuator functions that verifies the device supports setting the appropiate bits and if not return 0 immediately? > > More comments inline. > > > Signed-off-by: Logan Shaw <logan.shaw@alliedtelesis.co.nz> > > --- > > --- > > drivers/hwmon/adt7475.c | 163 > > +++++++++++++++++++++++++++++++++++++++- > > 1 file changed, 162 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c > > index 6c64d50c9aae..92a4be9e33eb 100644 > > --- a/drivers/hwmon/adt7475.c > > +++ b/drivers/hwmon/adt7475.c > > @@ -19,6 +19,7 @@ > > #include <linux/hwmon-vid.h> > > #include <linux/err.h> > > #include <linux/jiffies.h> > > +#include <linux/of.h> > > #include <linux/util_macros.h> > > > > /* Indexes for the sysfs hooks */ > > @@ -39,6 +40,7 @@ > > > > #define ALARM 9 > > #define FAULT 10 > > +#define ATTENUATE 11 > > > > /* 7475 Common Registers */ > > > > @@ -126,9 +128,11 @@ > > #define ADT7475_TACH_COUNT 4 > > #define ADT7475_PWM_COUNT 3 > > > > -/* Macro to read the registers */ > > +/* Macros to read and write registers */ > > > > #define adt7475_read(reg) i2c_smbus_read_byte_data(client, (reg)) > > +#define adt7475_write(reg, data) i2c_smbus_write_byte_data(client, > > (reg), \ > > + (data)) > > Those are quite pointless macros. Worse, they use a hidden parameter > which should be a no-go to start with. I would accept a patch to > remove > the first one, but I won't accept a patch adding yet another > pointless > macro. Your criticism is very valid, I will remove the new macro (and time dependent remove the other). > > > > > /* Macros to easily index the registers */ > > > > @@ -534,6 +538,82 @@ static ssize_t temp_store(struct device *dev, > > struct device_attribute *attr, > > return count; > > } > > > > +/** > > + * Gets the attenuator bit index for a sattr_index. > > + * > > + * If there is no attenuator bit index for a given sattr_index > > then returns > > + * a negative error code. > > + */ > > +static int config4_attenuate_index(char sattr_index) > > +{ > > + int index = -EBADR; > > + > > + switch (sattr_index) { > > + case 0: > > + index = 4; > > + break; > > + case 1: > > + index = 5; > > + break; > > + case 3: > > + index = 6; > > + break; > > + case 4: > > + index = 7; > > + break; > > + } > > + > > + return index; > > +} > > This function won't be called for non-supported values of > sattr_index. > The calling code doesn't even check the return value for errors. > This is unacceptable, and can be simplified with an if/else. > Besides, the the correct index could be set directly in > SENSOR_DEVICE_ATTR_2_RW, making the function quite pointless. > I was aiming for a "nice to have for somebody else in the future", however I can see that it is unwieldy and adds little value. Additionally it could cause undefined results if somebody does not check the return value before bit shifting by its negative error code value. I will remove this and hard code the four values. > > + > > +/** > > + * Gets the bypass attenuator bit for a voltage input and stores > > it in the char > > + * array buf. > > + */ > > +static ssize_t bypass_attenuator_show(struct device *dev, > > + struct device_attribute *attr, > > char *buf) > > +{ > > + struct adt7475_data *data = adt7475_update_device(dev); > > + struct sensor_device_attribute_2 *sattr = > > to_sensor_dev_attr_2(attr); > > + u8 attn_bypassed = !!(data->config4 & > > + (1 << config4_attenuate_index(sattr- > > >index))); > > + if (IS_ERR(data)) > > + return PTR_ERR(data); > > + > > + return sprintf(buf, "%d\n", attn_bypassed); > > +} > > + > > +/** > > + * Sets the bypass attenuator bit for a given voltage input. > > + */ > > +static ssize_t bypass_attenuator_store(struct device *dev, > > + struct device_attribute *attr, > > + const char *buf, size_t count) > > +{ > > + struct sensor_device_attribute_2 *sattr = > > to_sensor_dev_attr_2(attr); > > + struct i2c_client *client = to_i2c_client(dev); > > + struct adt7475_data *data = i2c_get_clientdata(client); > > + long val; > > + > > + if (kstrtol(buf, 2, &val)) > > + return -EINVAL; > > + > > + mutex_lock(&data->lock); > > + data->config4 = adt7475_read(REG_CONFIG4); > > + if (data->config4 < 0) > > + return data->config4; > > + > > + if (val == 0) > > + data->config4 &= ~(1 << config4_attenuate_index(sattr- > > >index)); > > + else > > + data->config4 |= (1 << config4_attenuate_index(sattr- > > >index)); > > + > > + adt7475_write(REG_CONFIG4, data->config4); > > + mutex_unlock(&data->lock); > > + > > + return count; > > +} > > + > > /* Assuming CONFIG6[SLOW] is 0 */ > > static const int ad7475_st_map[] = { > > 37500, 18800, 12500, 7500, 4700, 3100, 1600, 800, > > @@ -1080,10 +1160,14 @@ static SENSOR_DEVICE_ATTR_2_RO(in0_input, > > voltage, INPUT, 0); > > static SENSOR_DEVICE_ATTR_2_RW(in0_max, voltage, MAX, 0); > > static SENSOR_DEVICE_ATTR_2_RW(in0_min, voltage, MIN, 0); > > static SENSOR_DEVICE_ATTR_2_RO(in0_alarm, voltage, ALARM, 0); > > +static SENSOR_DEVICE_ATTR_2_RW(in0_attenuator_bypass, > > bypass_attenuator, > > + ATTENUATE, 0); > > static SENSOR_DEVICE_ATTR_2_RO(in1_input, voltage, INPUT, 1); > > static SENSOR_DEVICE_ATTR_2_RW(in1_max, voltage, MAX, 1); > > static SENSOR_DEVICE_ATTR_2_RW(in1_min, voltage, MIN, 1); > > static SENSOR_DEVICE_ATTR_2_RO(in1_alarm, voltage, ALARM, 1); > > +static SENSOR_DEVICE_ATTR_2_RW(in1_attenuator_bypass, > > bypass_attenuator, > > + ATTENUATE, 1); > > static SENSOR_DEVICE_ATTR_2_RO(in2_input, voltage, INPUT, 2); > > static SENSOR_DEVICE_ATTR_2_RW(in2_max, voltage, MAX, 2); > > static SENSOR_DEVICE_ATTR_2_RW(in2_min, voltage, MIN, 2); > > @@ -1092,10 +1176,14 @@ static SENSOR_DEVICE_ATTR_2_RO(in3_input, > > voltage, INPUT, 3); > > static SENSOR_DEVICE_ATTR_2_RW(in3_max, voltage, MAX, 3); > > static SENSOR_DEVICE_ATTR_2_RW(in3_min, voltage, MIN, 3); > > static SENSOR_DEVICE_ATTR_2_RO(in3_alarm, voltage, ALARM, 3); > > +static SENSOR_DEVICE_ATTR_2_RW(in3_attenuator_bypass, > > bypass_attenuator, > > + ATTENUATE, 3); > > static SENSOR_DEVICE_ATTR_2_RO(in4_input, voltage, INPUT, 4); > > static SENSOR_DEVICE_ATTR_2_RW(in4_max, voltage, MAX, 4); > > static SENSOR_DEVICE_ATTR_2_RW(in4_min, voltage, MIN, 4); > > static SENSOR_DEVICE_ATTR_2_RO(in4_alarm, voltage, ALARM, 8); > > +static SENSOR_DEVICE_ATTR_2_RW(in4_attenuator_bypass, > > bypass_attenuator, > > + ATTENUATE, 4); > > static SENSOR_DEVICE_ATTR_2_RO(in5_input, voltage, INPUT, 5); > > static SENSOR_DEVICE_ATTR_2_RW(in5_max, voltage, MAX, 5); > > static SENSOR_DEVICE_ATTR_2_RW(in5_min, voltage, MIN, 5); > > @@ -1177,6 +1265,7 @@ static struct attribute *adt7475_attrs[] = { > > &sensor_dev_attr_in1_max.dev_attr.attr, > > &sensor_dev_attr_in1_min.dev_attr.attr, > > &sensor_dev_attr_in1_alarm.dev_attr.attr, > > + &sensor_dev_attr_in1_attenuator_bypass.dev_attr.attr, > > &sensor_dev_attr_in2_input.dev_attr.attr, > > &sensor_dev_attr_in2_max.dev_attr.attr, > > &sensor_dev_attr_in2_min.dev_attr.attr, > > @@ -1263,6 +1352,7 @@ static struct attribute *in0_attrs[] = { > > &sensor_dev_attr_in0_max.dev_attr.attr, > > &sensor_dev_attr_in0_min.dev_attr.attr, > > &sensor_dev_attr_in0_alarm.dev_attr.attr, > > + &sensor_dev_attr_in0_attenuator_bypass.dev_attr.attr, > > NULL > > }; > > > > @@ -1271,6 +1361,7 @@ static struct attribute *in3_attrs[] = { > > &sensor_dev_attr_in3_max.dev_attr.attr, > > &sensor_dev_attr_in3_min.dev_attr.attr, > > &sensor_dev_attr_in3_alarm.dev_attr.attr, > > + &sensor_dev_attr_in3_attenuator_bypass.dev_attr.attr, > > NULL > > }; > > > > @@ -1279,6 +1370,7 @@ static struct attribute *in4_attrs[] = { > > &sensor_dev_attr_in4_max.dev_attr.attr, > > &sensor_dev_attr_in4_min.dev_attr.attr, > > &sensor_dev_attr_in4_alarm.dev_attr.attr, > > + &sensor_dev_attr_in4_attenuator_bypass.dev_attr.attr, > > NULL > > }; > > > > @@ -1457,6 +1549,69 @@ static int adt7475_update_limits(struct > > i2c_client *client) > > return 0; > > } > > > > +/** > > Is this an API function that would warrant documentation ? Yes it would. If you approve removing the sysfs additions (including the above) than that solves that. > > > + * Reads individual voltage input bypass attenuator properties > > from the DTS, > > + * and if the property is present the corresponding bit is set in > > the > > + * register. > > + * > > + * Properties must be in the form of "bypass-attenuator-inx", > > where x is an > > + * integer from the set {0, 1, 3, 4} (can not bypass in2 > > attenuator). > > + * > > + * Returns a negative error code if there was an error writing to > > the register. > > + */ > > +static int load_individual_bypass_attenuators(const struct > > i2c_client *client, > > + u8 *config4) > > +{ > > + char buf[32]; > > + int attenuate_index, input_index; > > + u8 config4_copy = *config4; > > + > > + for (input_index = 0; input_index < 5; input_index++) { > > + attenuate_index = config4_attenuate_index(input_index); > > + if (attenuate_index < 0) > > + continue; > > + > > + sprintf(buf, "bypass-attenuator-in%d", input_index); > > + if (of_get_property(client->dev.of_node, buf, NULL)) > > + config4_copy |= (1 << attenuate_index); > > + } > > + > > + if (adt7475_write(REG_CONFIG4, config4_copy) < 0) > > + // Failed to update register > > Please follow Documentation/hwmon/submitting-patches.rst. > > > + return -EREMOTEIO; > > + > > + *config4 = config4_copy; > > + > > + return 0; > > +} > > + > > +/** > > + * Sets the bypass all attenuators bit, if the "bypass-attenuator- > > all" > > + * property exists in the DTS. > > + * > > + * Returns a negative error code if there was an error writing to > > the > > + * register. > > + */ > > +static int load_all_bypass_attenuator(const struct i2c_client > > *client, > > + u8 *config2) > > +{ > > + u8 config2_copy = *config2; > > + > > + if (!of_get_property(client->dev.of_node, > > + "bypass-attenuator-all", NULL)) > > + return 0; > > + > > + config2_copy |= (1 << 5); > > + > > + if (adt7475_write(REG_CONFIG2, config2_copy) < 0) > > + // failed to write to register > > + return -EREMOTEIO; > > + > > + *config2 = config2_copy; > > + > > + return 0; > > +} > > + > > static int adt7475_probe(struct i2c_client *client, > > const struct i2c_device_id *id) > > { > > @@ -1545,7 +1700,13 @@ static int adt7475_probe(struct i2c_client > > *client, > > } > > > > /* Voltage attenuators can be bypassed, globally or > > individually */ > > + if (load_individual_bypass_attenuators(client, &(data- > > >config4)) < 0) > > + dev_warn(&client->dev, > > + "Error setting bypass attenuator bits\n"); > > + > > config2 = adt7475_read(REG_CONFIG2); > > + if (load_all_bypass_attenuator(client, &config2) < 0) > > + dev_warn(&client->dev, "Error setting bypass all > > attenuator\n"); > > if (config2 & CONFIG2_ATTN) { > > data->bypass_attn = (0x3 << 3) | 0x3; > > } else { > > > >
On Wed, Dec 18, 2019 at 04:30:39AM +0000, Logan Shaw wrote: [...] > > > > The datasheet for ADC7475 does not say anything about the ability to > > control > > attenuators other than for vcc in configuration register 4. Bit 4, 6, > > and 7 > > are listed as unused/reserved, suggesting that those bits - if at all > > - are > > only defined for other chips. Nothing in this patch suggests what > > those chips > > are. Attenuation bits need to be validated against the chip type. > > You are right, I missed including important details. The ADT7476 and > ADT7490 datasheets specify "Bits [7:4] of Configuration Register 4 > (0x7D) can be used to bypass individual voltage channel attenuators". > > My thought process was it would be up to the person configuring the > devicetree to only add the attributes where appropiate (for example, > not for a ADT7475 chip). I can see this is dangerious. Instead would it > be acceptable to add a check to the load_individual_bypass_attenuators > and load_all_bypass_attenuator functions that verifies the device > supports setting the appropiate bits and if not return 0 immediately? > Devicetree properties are acceptable, but not writing bits which are not supported (reserved) for a given chip. How to implement this is up to you. Based on your feedback, and my personal opinion, I won't accept new (non-standard) sysfs attributes. Note that I also won't accept C++ style comments in this driver. Guenter
diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c index 6c64d50c9aae..92a4be9e33eb 100644 --- a/drivers/hwmon/adt7475.c +++ b/drivers/hwmon/adt7475.c @@ -19,6 +19,7 @@ #include <linux/hwmon-vid.h> #include <linux/err.h> #include <linux/jiffies.h> +#include <linux/of.h> #include <linux/util_macros.h> /* Indexes for the sysfs hooks */ @@ -39,6 +40,7 @@ #define ALARM 9 #define FAULT 10 +#define ATTENUATE 11 /* 7475 Common Registers */ @@ -126,9 +128,11 @@ #define ADT7475_TACH_COUNT 4 #define ADT7475_PWM_COUNT 3 -/* Macro to read the registers */ +/* Macros to read and write registers */ #define adt7475_read(reg) i2c_smbus_read_byte_data(client, (reg)) +#define adt7475_write(reg, data) i2c_smbus_write_byte_data(client, (reg), \ + (data)) /* Macros to easily index the registers */ @@ -534,6 +538,82 @@ static ssize_t temp_store(struct device *dev, struct device_attribute *attr, return count; } +/** + * Gets the attenuator bit index for a sattr_index. + * + * If there is no attenuator bit index for a given sattr_index then returns + * a negative error code. + */ +static int config4_attenuate_index(char sattr_index) +{ + int index = -EBADR; + + switch (sattr_index) { + case 0: + index = 4; + break; + case 1: + index = 5; + break; + case 3: + index = 6; + break; + case 4: + index = 7; + break; + } + + return index; +} + +/** + * Gets the bypass attenuator bit for a voltage input and stores it in the char + * array buf. + */ +static ssize_t bypass_attenuator_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct adt7475_data *data = adt7475_update_device(dev); + struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr); + u8 attn_bypassed = !!(data->config4 & + (1 << config4_attenuate_index(sattr->index))); + if (IS_ERR(data)) + return PTR_ERR(data); + + return sprintf(buf, "%d\n", attn_bypassed); +} + +/** + * Sets the bypass attenuator bit for a given voltage input. + */ +static ssize_t bypass_attenuator_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct sensor_device_attribute_2 *sattr = to_sensor_dev_attr_2(attr); + struct i2c_client *client = to_i2c_client(dev); + struct adt7475_data *data = i2c_get_clientdata(client); + long val; + + if (kstrtol(buf, 2, &val)) + return -EINVAL; + + mutex_lock(&data->lock); + data->config4 = adt7475_read(REG_CONFIG4); + if (data->config4 < 0) + return data->config4; + + if (val == 0) + data->config4 &= ~(1 << config4_attenuate_index(sattr->index)); + else + data->config4 |= (1 << config4_attenuate_index(sattr->index)); + + adt7475_write(REG_CONFIG4, data->config4); + mutex_unlock(&data->lock); + + return count; +} + /* Assuming CONFIG6[SLOW] is 0 */ static const int ad7475_st_map[] = { 37500, 18800, 12500, 7500, 4700, 3100, 1600, 800, @@ -1080,10 +1160,14 @@ static SENSOR_DEVICE_ATTR_2_RO(in0_input, voltage, INPUT, 0); static SENSOR_DEVICE_ATTR_2_RW(in0_max, voltage, MAX, 0); static SENSOR_DEVICE_ATTR_2_RW(in0_min, voltage, MIN, 0); static SENSOR_DEVICE_ATTR_2_RO(in0_alarm, voltage, ALARM, 0); +static SENSOR_DEVICE_ATTR_2_RW(in0_attenuator_bypass, bypass_attenuator, + ATTENUATE, 0); static SENSOR_DEVICE_ATTR_2_RO(in1_input, voltage, INPUT, 1); static SENSOR_DEVICE_ATTR_2_RW(in1_max, voltage, MAX, 1); static SENSOR_DEVICE_ATTR_2_RW(in1_min, voltage, MIN, 1); static SENSOR_DEVICE_ATTR_2_RO(in1_alarm, voltage, ALARM, 1); +static SENSOR_DEVICE_ATTR_2_RW(in1_attenuator_bypass, bypass_attenuator, + ATTENUATE, 1); static SENSOR_DEVICE_ATTR_2_RO(in2_input, voltage, INPUT, 2); static SENSOR_DEVICE_ATTR_2_RW(in2_max, voltage, MAX, 2); static SENSOR_DEVICE_ATTR_2_RW(in2_min, voltage, MIN, 2); @@ -1092,10 +1176,14 @@ static SENSOR_DEVICE_ATTR_2_RO(in3_input, voltage, INPUT, 3); static SENSOR_DEVICE_ATTR_2_RW(in3_max, voltage, MAX, 3); static SENSOR_DEVICE_ATTR_2_RW(in3_min, voltage, MIN, 3); static SENSOR_DEVICE_ATTR_2_RO(in3_alarm, voltage, ALARM, 3); +static SENSOR_DEVICE_ATTR_2_RW(in3_attenuator_bypass, bypass_attenuator, + ATTENUATE, 3); static SENSOR_DEVICE_ATTR_2_RO(in4_input, voltage, INPUT, 4); static SENSOR_DEVICE_ATTR_2_RW(in4_max, voltage, MAX, 4); static SENSOR_DEVICE_ATTR_2_RW(in4_min, voltage, MIN, 4); static SENSOR_DEVICE_ATTR_2_RO(in4_alarm, voltage, ALARM, 8); +static SENSOR_DEVICE_ATTR_2_RW(in4_attenuator_bypass, bypass_attenuator, + ATTENUATE, 4); static SENSOR_DEVICE_ATTR_2_RO(in5_input, voltage, INPUT, 5); static SENSOR_DEVICE_ATTR_2_RW(in5_max, voltage, MAX, 5); static SENSOR_DEVICE_ATTR_2_RW(in5_min, voltage, MIN, 5); @@ -1177,6 +1265,7 @@ static struct attribute *adt7475_attrs[] = { &sensor_dev_attr_in1_max.dev_attr.attr, &sensor_dev_attr_in1_min.dev_attr.attr, &sensor_dev_attr_in1_alarm.dev_attr.attr, + &sensor_dev_attr_in1_attenuator_bypass.dev_attr.attr, &sensor_dev_attr_in2_input.dev_attr.attr, &sensor_dev_attr_in2_max.dev_attr.attr, &sensor_dev_attr_in2_min.dev_attr.attr, @@ -1263,6 +1352,7 @@ static struct attribute *in0_attrs[] = { &sensor_dev_attr_in0_max.dev_attr.attr, &sensor_dev_attr_in0_min.dev_attr.attr, &sensor_dev_attr_in0_alarm.dev_attr.attr, + &sensor_dev_attr_in0_attenuator_bypass.dev_attr.attr, NULL }; @@ -1271,6 +1361,7 @@ static struct attribute *in3_attrs[] = { &sensor_dev_attr_in3_max.dev_attr.attr, &sensor_dev_attr_in3_min.dev_attr.attr, &sensor_dev_attr_in3_alarm.dev_attr.attr, + &sensor_dev_attr_in3_attenuator_bypass.dev_attr.attr, NULL }; @@ -1279,6 +1370,7 @@ static struct attribute *in4_attrs[] = { &sensor_dev_attr_in4_max.dev_attr.attr, &sensor_dev_attr_in4_min.dev_attr.attr, &sensor_dev_attr_in4_alarm.dev_attr.attr, + &sensor_dev_attr_in4_attenuator_bypass.dev_attr.attr, NULL }; @@ -1457,6 +1549,69 @@ static int adt7475_update_limits(struct i2c_client *client) return 0; } +/** + * Reads individual voltage input bypass attenuator properties from the DTS, + * and if the property is present the corresponding bit is set in the + * register. + * + * Properties must be in the form of "bypass-attenuator-inx", where x is an + * integer from the set {0, 1, 3, 4} (can not bypass in2 attenuator). + * + * Returns a negative error code if there was an error writing to the register. + */ +static int load_individual_bypass_attenuators(const struct i2c_client *client, + u8 *config4) +{ + char buf[32]; + int attenuate_index, input_index; + u8 config4_copy = *config4; + + for (input_index = 0; input_index < 5; input_index++) { + attenuate_index = config4_attenuate_index(input_index); + if (attenuate_index < 0) + continue; + + sprintf(buf, "bypass-attenuator-in%d", input_index); + if (of_get_property(client->dev.of_node, buf, NULL)) + config4_copy |= (1 << attenuate_index); + } + + if (adt7475_write(REG_CONFIG4, config4_copy) < 0) + // Failed to update register + return -EREMOTEIO; + + *config4 = config4_copy; + + return 0; +} + +/** + * Sets the bypass all attenuators bit, if the "bypass-attenuator-all" + * property exists in the DTS. + * + * Returns a negative error code if there was an error writing to the + * register. + */ +static int load_all_bypass_attenuator(const struct i2c_client *client, + u8 *config2) +{ + u8 config2_copy = *config2; + + if (!of_get_property(client->dev.of_node, + "bypass-attenuator-all", NULL)) + return 0; + + config2_copy |= (1 << 5); + + if (adt7475_write(REG_CONFIG2, config2_copy) < 0) + // failed to write to register + return -EREMOTEIO; + + *config2 = config2_copy; + + return 0; +} + static int adt7475_probe(struct i2c_client *client, const struct i2c_device_id *id) { @@ -1545,7 +1700,13 @@ static int adt7475_probe(struct i2c_client *client, } /* Voltage attenuators can be bypassed, globally or individually */ + if (load_individual_bypass_attenuators(client, &(data->config4)) < 0) + dev_warn(&client->dev, + "Error setting bypass attenuator bits\n"); + config2 = adt7475_read(REG_CONFIG2); + if (load_all_bypass_attenuator(client, &config2) < 0) + dev_warn(&client->dev, "Error setting bypass all attenuator\n"); if (config2 & CONFIG2_ATTN) { data->bypass_attn = (0x3 << 3) | 0x3; } else {
Added support for reading and writing the individual and global voltage attenuator bypasses. Added loading DTS properties to bypass attenuators on device probe. Signed-off-by: Logan Shaw <logan.shaw@alliedtelesis.co.nz> --- --- drivers/hwmon/adt7475.c | 163 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 162 insertions(+), 1 deletion(-)