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