[PATCH 0/1] hwmon: (adt7475) Added attenuator bypass support

[PATCH 0/1] hwmon: (adt7475) Added attenuator bypass support

[Logan Shaw]

The ADT7476 controller supports bypassing all voltage input attenuators
and bypassing individual voltage input attenuators. This can be used to
improve the resolution of low voltages. Presently the driver does not
support this functionality, and this patch adds it.

Logan Shaw (1):
  hwmon: (adt7475) Added attenuator bypass support

 drivers/hwmon/adt7475.c | 162 +++++++++++++++++++++++++++++++++++++++-
 1 file changed, 161 insertions(+), 1 deletion(-)

-- 
2.23.0

[PATCH 1/1] hwmon: (adt7475) Added attenuator bypass support

[Logan Shaw]

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(-)

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 {
-- 
2.23.0

Re: [PATCH 1/1] hwmon: (adt7475) Added attenuator bypass support

[Guenter Roeck]

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 {
> 

Re: [PATCH 1/1] hwmon: (adt7475) Added attenuator bypass support

[Logan Shaw]

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 {
> > 
> 
> 

Re: [PATCH 1/1] hwmon: (adt7475) Added attenuator bypass support

[Guenter Roeck]

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