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

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

[Logan Shaw]

The ADT7473 and ADT7475 support bypassing voltage input attenuators on
voltage input 1 and the ADT7476 and ADT7490 additionally support
bypassing voltage input attenuators on voltage inputs 0, 3, and 4. This
can be useful to improve measurement resolution when measuring voltages
0 V - 2.25 V.

This patch adds 4 optional devicetree properties to the adt7475
driver, each setting the attenuator bypass (or clearing) on a
specific voltage input.

* v6
- removed unnecessary comments marked for kernal documentation in file
  adt7475.c.
- fixed parenthesis whitespace allignment in multiple places in file
  adt7475.c.
- removed unnecessary parentheses around data->config4 in file
  adt7475.c.
- removed unused assignment "int i = 0" in file adt7475.c.
- fixed mispelling of documentation in commit subject.

* v5
- modified adt7475.yaml to remove dt_binding_check errors
- made various alignment fixes to adt7475.c to improve style.
- renamed function modify_config_from_dts_prop to modify_config
- changed return type of function modify_config to void
- function modify_config error return code modified no longer override
  the dependent functions i2c_smbus_write_byte_data error.
- renamed function load_all_bypass_attenuators to load_attenuators
- in function load_attenuators the two local variables config2_copy
  and config4_copy have been combined into one: conf_copy.

* v4
- fixed a small error in file adt7475.yaml (duplicate property names).

* v3
- removed the functionality to set the global attenuator bypass.
- added functionality to allow bypassing voltage input 1 on the
	ADT7473 and ADT7475.
- added DTS definition file adt7475.yaml and 4 new properties.
- added the previousely missing newline character to the end of
  	file adt7475.c. 

* v2
- removed sysfs changes from patch
- removed adt7475_write macro from patch and replaced it by using
	the i2c_smbus_write_byte_data function directly in code.
- removed config4_attenuate_index function from patch and replaced it
	by modifying the function  load_individual_bypass_attenuators
	to use hard coded bit values.
- modified function load_individual_bypass_attenuators to use 4 if
	statements, one for each voltage input, replacing the for loop.
- modified function adt7475_probe to check the device is a ADT7476 or
	ADT7490 (other devices do not support bypassing all or
	individual attenuators), and only then set the relevant bits.
- added new file adt7475.txt to document the new devicetree properties.
- removed c++ style comments.

Logan Shaw (2):
  hwmon: (adt7475) Added attenuator bypass support
  dt-bindings: hwmon: (adt7475) Added missing adt7475 documentation

 .../devicetree/bindings/hwmon/adt7475.yaml    | 95 +++++++++++++++++++
 drivers/hwmon/adt7475.c                       | 55 +++++++++++
 2 files changed, 150 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/adt7475.yaml

-- 
2.25.0

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

[Logan Shaw]

Added support for reading DTS properties to set attenuators on
device probe for the ADT7473, ADT7475, ADT7476, and ADT7490.

Signed-off-by: Logan Shaw <logan.shaw@alliedtelesis.co.nz>
---
---
 drivers/hwmon/adt7475.c | 55 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/drivers/hwmon/adt7475.c b/drivers/hwmon/adt7475.c
index 6c64d50c9aae..3f4bdc1eaaec 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 */
@@ -1457,6 +1458,57 @@ static int adt7475_update_limits(struct i2c_client *client)
 	return 0;
 }
 
+static void modify_config(const struct i2c_client *client, char *property,
+			  u8 *config, u8 bit_index)
+{
+	u32 prop_value = 0;
+	int ret = of_property_read_u32(client->dev.of_node, property,
+					&prop_value);
+
+	if (!ret) {
+		if (prop_value)
+			*config |= (1 << bit_index);
+		else
+			*config &= ~(1 << bit_index);
+	}
+}
+
+static int load_attenuators(const struct i2c_client *client, int chip,
+			    u8 *config2, u8 *config4)
+{
+	u8 conf_copy;
+	int ret;
+
+	if (chip == adt7476 || chip == adt7490) {
+		conf_copy = *config4;
+
+		modify_config(client, "bypass-attenuator-in0", &conf_copy, 4);
+		modify_config(client, "bypass-attenuator-in1", &conf_copy, 5);
+		modify_config(client, "bypass-attenuator-in3", &conf_copy, 6);
+		modify_config(client, "bypass-attenuator-in4", &conf_copy, 7);
+
+		ret = i2c_smbus_write_byte_data(client, REG_CONFIG4,
+						conf_copy);
+		if (ret < 0)
+			return ret;
+
+		*config4 = conf_copy;
+	} else if (chip == adt7473 || chip == adt7475) {
+		conf_copy = *config2;
+
+		modify_config(client, "bypass-attenuator-in1", &conf_copy, 5);
+
+		ret = i2c_smbus_write_byte_data(client, REG_CONFIG2,
+						conf_copy);
+		if (ret < 0)
+			return ret;
+
+		*config2 = conf_copy;
+	}
+
+	return 0;
+}
+
 static int adt7475_probe(struct i2c_client *client,
 			 const struct i2c_device_id *id)
 {
@@ -1546,6 +1598,9 @@ static int adt7475_probe(struct i2c_client *client,
 
 	/* Voltage attenuators can be bypassed, globally or individually */
 	config2 = adt7475_read(REG_CONFIG2);
+	if (load_attenuators(client, chip, &config2, &data->config4) < 0)
+		dev_warn(&client->dev, "Error setting bypass attenuators\n");
+
 	if (config2 & CONFIG2_ATTN) {
 		data->bypass_attn = (0x3 << 3) | 0x3;
 	} else {
-- 
2.25.0

[PATCH v6 2/2] dt-bindings: hwmon: (adt7475) Added missing adt7475 documentation

[Logan Shaw]

Added a new file documenting the adt7475 devicetree and added the four
new properties to it.

Signed-off-by: Logan Shaw <logan.shaw@alliedtelesis.co.nz>
---
---
 .../devicetree/bindings/hwmon/adt7475.yaml    | 95 +++++++++++++++++++
 1 file changed, 95 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/hwmon/adt7475.yaml

diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
new file mode 100644
index 000000000000..450da5e66e07
--- /dev/null
+++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
@@ -0,0 +1,95 @@
+# SPDX-License-Identifier: GPL-2.0
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/adt7475.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: ADT7475 hwmon sensor
+
+maintainers:
+  - Jean Delvare <jdelvare@suse.com>
+
+description: |
+  The ADT7473, ADT7475, ADT7476, and ADT7490 are thermal monitors and multiple
+  PWN fan controllers.
+
+  They support monitoring and controlling up to four fans (the ADT7490 can only
+  control up to three). They support reading a single on chip temperature
+  sensor and two off chip temperature sensors (the ADT7490 additionally
+  supports measuring up to three current external temperature sensors with
+  series resistance cancellation (SRC)).
+
+  Datasheets:
+  https://www.onsemi.com/pub/Collateral/ADT7473-D.PDF
+  https://www.onsemi.com/pub/Collateral/ADT7475-D.PDF
+  https://www.onsemi.com/pub/Collateral/ADT7476-D.PDF
+  https://www.onsemi.com/pub/Collateral/ADT7490-D.PDF
+
+  Description taken from omsemiconductors specification sheets, with minor
+  rephrasing.
+
+properties:
+  compatible:
+    enum:
+      - adi,adt7473
+      - adi,adt7475
+      - adi,adt7476
+      - adi,adt7490
+
+  reg:
+    maxItems: 1
+
+  bypass-attenuator-in0:
+    description: |
+      Configures bypassing the individual voltage input
+      attenuator, on in0. This is supported on the ADT7476 and ADT7490.
+      If set to a non-zero integer the attenuator is bypassed, if set to
+      zero the attenuator is not bypassed. If the property is absent then
+      the config register is not modified.
+    maxItems: 1
+
+  bypass-attenuator-in1:
+    description: |
+      Configures bypassing the individual voltage input
+      attenuator, on in1. This is supported on the ADT7473, ADT7475,
+      ADT7476 and ADT7490. If set to a non-zero integer the attenuator
+      is bypassed, if set to zero the attenuator is not bypassed. If the
+      property is absent then the config register is not modified.
+    maxItems: 1
+
+  bypass-attenuator-in3:
+    description: |
+      Configures bypassing the individual voltage input
+      attenuator, on in3. This is supported on the ADT7476 and ADT7490.
+      If set to a non-zero integer the attenuator is bypassed, if set to
+      zero the attenuator is not bypassed. If the property is absent then
+      the config register is not modified.
+    maxItems: 1
+
+  bypass-attenuator-in4:
+    description: |
+      Configures bypassing the individual voltage input
+      attenuator, on in4. This is supported on the ADT7476 and ADT7490.
+      If set to a non-zero integer the attenuator is bypassed, if set to
+      zero the attenuator is not bypassed. If the property is absent then
+      the config register is not modified.
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+
+examples:
+  - |
+    i2c {
+      #address-cells = <1>;
+      #size-cells = <0>;
+
+      hwmon@2e {
+        compatible = "adi,adt7476";
+        reg = <0x2e>;
+        bypass-attenuator-in0 = <1>;
+        bypass-attenuator-in1 = <0>;
+      };
+    };
+...
-- 
2.25.0

Re: [PATCH v6 2/2] dt-bindings: hwmon: (adt7475) Added missing adt7475 documentation

[Rob Herring]

On Mon, Jan 27, 2020 at 11:10:14AM +1300, Logan Shaw wrote:
> Added a new file documenting the adt7475 devicetree and added the four
> new properties to it.
> 
> Signed-off-by: Logan Shaw <logan.shaw@alliedtelesis.co.nz>
> ---
> ---
>  .../devicetree/bindings/hwmon/adt7475.yaml    | 95 +++++++++++++++++++
>  1 file changed, 95 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/hwmon/adt7475.yaml
> 
> diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> new file mode 100644
> index 000000000000..450da5e66e07
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> @@ -0,0 +1,95 @@
> +# SPDX-License-Identifier: GPL-2.0

Dual license new bindings please:

(GPL-2.0-only OR BSD-2-Clause)

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/adt7475.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: ADT7475 hwmon sensor
> +
> +maintainers:
> +  - Jean Delvare <jdelvare@suse.com>
> +
> +description: |
> +  The ADT7473, ADT7475, ADT7476, and ADT7490 are thermal monitors and multiple
> +  PWN fan controllers.
> +
> +  They support monitoring and controlling up to four fans (the ADT7490 can only
> +  control up to three). They support reading a single on chip temperature
> +  sensor and two off chip temperature sensors (the ADT7490 additionally
> +  supports measuring up to three current external temperature sensors with
> +  series resistance cancellation (SRC)).
> +
> +  Datasheets:
> +  https://www.onsemi.com/pub/Collateral/ADT7473-D.PDF
> +  https://www.onsemi.com/pub/Collateral/ADT7475-D.PDF
> +  https://www.onsemi.com/pub/Collateral/ADT7476-D.PDF
> +  https://www.onsemi.com/pub/Collateral/ADT7490-D.PDF
> +
> +  Description taken from omsemiconductors specification sheets, with minor

omsemi?
 ^

> +  rephrasing.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,adt7473
> +      - adi,adt7475
> +      - adi,adt7476
> +      - adi,adt7490
> +
> +  reg:
> +    maxItems: 1
> +
> +  bypass-attenuator-in0:

Needs a vendor prefix and a type ref. 

> +    description: |
> +      Configures bypassing the individual voltage input
> +      attenuator, on in0. This is supported on the ADT7476 and ADT7490.
> +      If set to a non-zero integer the attenuator is bypassed, if set to
> +      zero the attenuator is not bypassed. If the property is absent then
> +      the config register is not modified.

Sounds like this could be boolean? If not, define a schema for what are 
valid values.

> +    maxItems: 1
> +
> +  bypass-attenuator-in1:
> +    description: |
> +      Configures bypassing the individual voltage input
> +      attenuator, on in1. This is supported on the ADT7473, ADT7475,
> +      ADT7476 and ADT7490. If set to a non-zero integer the attenuator
> +      is bypassed, if set to zero the attenuator is not bypassed. If the
> +      property is absent then the config register is not modified.
> +    maxItems: 1
> +
> +  bypass-attenuator-in3:
> +    description: |
> +      Configures bypassing the individual voltage input
> +      attenuator, on in3. This is supported on the ADT7476 and ADT7490.
> +      If set to a non-zero integer the attenuator is bypassed, if set to
> +      zero the attenuator is not bypassed. If the property is absent then
> +      the config register is not modified.
> +    maxItems: 1
> +
> +  bypass-attenuator-in4:

These 4 could be a single entry under patternProperties.


> +    description: |
> +      Configures bypassing the individual voltage input
> +      attenuator, on in4. This is supported on the ADT7476 and ADT7490.
> +      If set to a non-zero integer the attenuator is bypassed, if set to
> +      zero the attenuator is not bypassed. If the property is absent then
> +      the config register is not modified.
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +
> +examples:
> +  - |
> +    i2c {
> +      #address-cells = <1>;
> +      #size-cells = <0>;
> +
> +      hwmon@2e {
> +        compatible = "adi,adt7476";
> +        reg = <0x2e>;
> +        bypass-attenuator-in0 = <1>;
> +        bypass-attenuator-in1 = <0>;
> +      };
> +    };
> +...
> -- 
> 2.25.0
> 

Re: [PATCH v6 2/2] dt-bindings: hwmon: (adt7475) Added missing adt7475 documentation

[Logan Shaw]

On Mon, 2020-01-27 at 09:48 -0600, Rob Herring wrote:
> On Mon, Jan 27, 2020 at 11:10:14AM +1300, Logan Shaw wrote:
> > Added a new file documenting the adt7475 devicetree and added the
> > four
> > new properties to it.
> > 
> > Signed-off-by: Logan Shaw <logan.shaw@alliedtelesis.co.nz>
> > ---
> > ---
> >  .../devicetree/bindings/hwmon/adt7475.yaml    | 95
> > +++++++++++++++++++
> >  1 file changed, 95 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/hwmon/adt7475.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> > b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> > new file mode 100644
> > index 000000000000..450da5e66e07
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> > @@ -0,0 +1,95 @@
> > +# SPDX-License-Identifier: GPL-2.0
> 
> Dual license new bindings please:
> 
> (GPL-2.0-only OR BSD-2-Clause)
> 
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/adt7475.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: ADT7475 hwmon sensor
> > +
> > +maintainers:
> > +  - Jean Delvare <jdelvare@suse.com>
> > +
> > +description: |
> > +  The ADT7473, ADT7475, ADT7476, and ADT7490 are thermal monitors
> > and multiple
> > +  PWN fan controllers.
> > +
> > +  They support monitoring and controlling up to four fans (the
> > ADT7490 can only
> > +  control up to three). They support reading a single on chip
> > temperature
> > +  sensor and two off chip temperature sensors (the ADT7490
> > additionally
> > +  supports measuring up to three current external temperature
> > sensors with
> > +  series resistance cancellation (SRC)).
> > +
> > +  Datasheets:
> > +  https://www.onsemi.com/pub/Collateral/ADT7473-D.PDF
> > +  https://www.onsemi.com/pub/Collateral/ADT7475-D.PDF
> > +  https://www.onsemi.com/pub/Collateral/ADT7476-D.PDF
> > +  https://www.onsemi.com/pub/Collateral/ADT7490-D.PDF
> > +
> > +  Description taken from omsemiconductors specification sheets,
> > with minor
> 
> omsemi?
>  ^
> 
> > +  rephrasing.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - adi,adt7473
> > +      - adi,adt7475
> > +      - adi,adt7476
> > +      - adi,adt7490
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  bypass-attenuator-in0:
> 
> Needs a vendor prefix and a type ref. 

Adi (Analog Devices) sold the ADT product line (amongst other things)
to On Semiconductor. As changing the vendor of these chips (in code)
would break backwards compatibility should we keep the vendor as adi?

To confirm, would this make the property "adi,adt7476,bypass-
attenuator-in0"?

So used in conjunction with patternProperties you would end up with
something like:

"adi,(adt7473|adt7475|adt7476|adt7490),bypass-attenuator-in[0134]"

> 
> > +    description: |
> > +      Configures bypassing the individual voltage input
> > +      attenuator, on in0. This is supported on the ADT7476 and
> > ADT7490.
> > +      If set to a non-zero integer the attenuator is bypassed, if
> > set to
> > +      zero the attenuator is not bypassed. If the property is
> > absent then
> > +      the config register is not modified.
> 
> Sounds like this could be boolean? If not, define a schema for what
> are 
> valid values.
> 
> > +    maxItems: 1
> > +
> > +  bypass-attenuator-in1:
> > +    description: |
> > +      Configures bypassing the individual voltage input
> > +      attenuator, on in1. This is supported on the ADT7473,
> > ADT7475,
> > +      ADT7476 and ADT7490. If set to a non-zero integer the
> > attenuator
> > +      is bypassed, if set to zero the attenuator is not bypassed.
> > If the
> > +      property is absent then the config register is not modified.
> > +    maxItems: 1
> > +
> > +  bypass-attenuator-in3:
> > +    description: |
> > +      Configures bypassing the individual voltage input
> > +      attenuator, on in3. This is supported on the ADT7476 and
> > ADT7490.
> > +      If set to a non-zero integer the attenuator is bypassed, if
> > set to
> > +      zero the attenuator is not bypassed. If the property is
> > absent then
> > +      the config register is not modified.
> > +    maxItems: 1
> > +
> > +  bypass-attenuator-in4:
> 
> These 4 could be a single entry under patternProperties.
> 
> 
> > +    description: |
> > +      Configures bypassing the individual voltage input
> > +      attenuator, on in4. This is supported on the ADT7476 and
> > ADT7490.
> > +      If set to a non-zero integer the attenuator is bypassed, if
> > set to
> > +      zero the attenuator is not bypassed. If the property is
> > absent then
> > +      the config register is not modified.
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +
> > +      hwmon@2e {
> > +        compatible = "adi,adt7476";
> > +        reg = <0x2e>;
> > +        bypass-attenuator-in0 = <1>;
> > +        bypass-attenuator-in1 = <0>;
> > +      };
> > +    };
> > +...
> > -- 
> > 2.25.0
> > 

Re: [PATCH v6 2/2] dt-bindings: hwmon: (adt7475) Added missing adt7475 documentation

[Guenter Roeck]

On 1/28/20 8:30 PM, Logan Shaw wrote:
> On Mon, 2020-01-27 at 09:48 -0600, Rob Herring wrote:
>> On Mon, Jan 27, 2020 at 11:10:14AM +1300, Logan Shaw wrote:
>>> Added a new file documenting the adt7475 devicetree and added the
>>> four
>>> new properties to it.
>>>
>>> Signed-off-by: Logan Shaw <logan.shaw@alliedtelesis.co.nz>
>>> ---
>>> ---
>>>   .../devicetree/bindings/hwmon/adt7475.yaml    | 95
>>> +++++++++++++++++++
>>>   1 file changed, 95 insertions(+)
>>>   create mode 100644
>>> Documentation/devicetree/bindings/hwmon/adt7475.yaml
>>>
>>> diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml
>>> b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
>>> new file mode 100644
>>> index 000000000000..450da5e66e07
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
>>> @@ -0,0 +1,95 @@
>>> +# SPDX-License-Identifier: GPL-2.0
>>
>> Dual license new bindings please:
>>
>> (GPL-2.0-only OR BSD-2-Clause)
>>
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/adt7475.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: ADT7475 hwmon sensor
>>> +
>>> +maintainers:
>>> +  - Jean Delvare <jdelvare@suse.com>
>>> +
>>> +description: |
>>> +  The ADT7473, ADT7475, ADT7476, and ADT7490 are thermal monitors
>>> and multiple
>>> +  PWN fan controllers.
>>> +
>>> +  They support monitoring and controlling up to four fans (the
>>> ADT7490 can only
>>> +  control up to three). They support reading a single on chip
>>> temperature
>>> +  sensor and two off chip temperature sensors (the ADT7490
>>> additionally
>>> +  supports measuring up to three current external temperature
>>> sensors with
>>> +  series resistance cancellation (SRC)).
>>> +
>>> +  Datasheets:
>>> +  https://www.onsemi.com/pub/Collateral/ADT7473-D.PDF
>>> +  https://www.onsemi.com/pub/Collateral/ADT7475-D.PDF
>>> +  https://www.onsemi.com/pub/Collateral/ADT7476-D.PDF
>>> +  https://www.onsemi.com/pub/Collateral/ADT7490-D.PDF
>>> +
>>> +  Description taken from omsemiconductors specification sheets,
>>> with minor
>>
>> omsemi?
>>   ^
>>
>>> +  rephrasing.
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - adi,adt7473
>>> +      - adi,adt7475
>>> +      - adi,adt7476
>>> +      - adi,adt7490
>>> +
>>> +  reg:
>>> +    maxItems: 1
>>> +
>>> +  bypass-attenuator-in0:
>>
>> Needs a vendor prefix and a type ref.
> 
> Adi (Analog Devices) sold the ADT product line (amongst other things)
> to On Semiconductor. As changing the vendor of these chips (in code)
> would break backwards compatibility should we keep the vendor as adi?
> 
> To confirm, would this make the property "adi,adt7476,bypass-
> attenuator-in0"?
> 
> So used in conjunction with patternProperties you would end up with
> something like:
> 
> "adi,(adt7473|adt7475|adt7476|adt7490),bypass-attenuator-in[0134]"
> 

That seems highly unusual and would be quite messy to implement.
I don't see the point of having the chip name in individual properties.

Guenter

>>
>>> +    description: |
>>> +      Configures bypassing the individual voltage input
>>> +      attenuator, on in0. This is supported on the ADT7476 and
>>> ADT7490.
>>> +      If set to a non-zero integer the attenuator is bypassed, if
>>> set to
>>> +      zero the attenuator is not bypassed. If the property is
>>> absent then
>>> +      the config register is not modified.
>>
>> Sounds like this could be boolean? If not, define a schema for what
>> are
>> valid values.
>>
>>> +    maxItems: 1
>>> +
>>> +  bypass-attenuator-in1:
>>> +    description: |
>>> +      Configures bypassing the individual voltage input
>>> +      attenuator, on in1. This is supported on the ADT7473,
>>> ADT7475,
>>> +      ADT7476 and ADT7490. If set to a non-zero integer the
>>> attenuator
>>> +      is bypassed, if set to zero the attenuator is not bypassed.
>>> If the
>>> +      property is absent then the config register is not modified.
>>> +    maxItems: 1
>>> +
>>> +  bypass-attenuator-in3:
>>> +    description: |
>>> +      Configures bypassing the individual voltage input
>>> +      attenuator, on in3. This is supported on the ADT7476 and
>>> ADT7490.
>>> +      If set to a non-zero integer the attenuator is bypassed, if
>>> set to
>>> +      zero the attenuator is not bypassed. If the property is
>>> absent then
>>> +      the config register is not modified.
>>> +    maxItems: 1
>>> +
>>> +  bypass-attenuator-in4:
>>
>> These 4 could be a single entry under patternProperties.
>>
>>
>>> +    description: |
>>> +      Configures bypassing the individual voltage input
>>> +      attenuator, on in4. This is supported on the ADT7476 and
>>> ADT7490.
>>> +      If set to a non-zero integer the attenuator is bypassed, if
>>> set to
>>> +      zero the attenuator is not bypassed. If the property is
>>> absent then
>>> +      the config register is not modified.
>>> +    maxItems: 1
>>> +
>>> +required:
>>> +  - compatible
>>> +  - reg
>>> +
>>> +examples:
>>> +  - |
>>> +    i2c {
>>> +      #address-cells = <1>;
>>> +      #size-cells = <0>;
>>> +
>>> +      hwmon@2e {
>>> +        compatible = "adi,adt7476";
>>> +        reg = <0x2e>;
>>> +        bypass-attenuator-in0 = <1>;
>>> +        bypass-attenuator-in1 = <0>;
>>> +      };
>>> +    };
>>> +...
>>> -- 
>>> 2.25.0
>>>

Re: [PATCH v6 2/2] dt-bindings: hwmon: (adt7475) Added missing adt7475 documentation

[Rob Herring]

On Tue, Jan 28, 2020 at 10:30 PM Logan Shaw
<Logan.Shaw@alliedtelesis.co.nz> wrote:
>
> On Mon, 2020-01-27 at 09:48 -0600, Rob Herring wrote:
> > On Mon, Jan 27, 2020 at 11:10:14AM +1300, Logan Shaw wrote:
> > > Added a new file documenting the adt7475 devicetree and added the
> > > four
> > > new properties to it.
> > >
> > > Signed-off-by: Logan Shaw <logan.shaw@alliedtelesis.co.nz>
> > > ---

> > > +  bypass-attenuator-in0:
> >
> > Needs a vendor prefix and a type ref.
>
> Adi (Analog Devices) sold the ADT product line (amongst other things)
> to On Semiconductor. As changing the vendor of these chips (in code)
> would break backwards compatibility should we keep the vendor as adi?

Yes. It should match what's used in the compatible string(s).

> To confirm, would this make the property "adi,adt7476,bypass-
> attenuator-in0"?
>
> So used in conjunction with patternProperties you would end up with
> something like:
>
> "adi,(adt7473|adt7475|adt7476|adt7490),bypass-attenuator-in[0134]"

No for the part #'s. Just add 'adi,'. Maybe you thought for type ref
that's what I meant? A type ref is:

$ref: /schemas/types.yaml#/definitions/uint32

Re: [PATCH v6 2/2] dt-bindings: hwmon: (adt7475) Added missing adt7475 documentation

[Logan Shaw]

On Wed, 2020-01-29 at 11:27 -0600, Rob Herring wrote:
> On Tue, Jan 28, 2020 at 10:30 PM Logan Shaw
> <Logan.Shaw@alliedtelesis.co.nz> wrote:
> > 
> > On Mon, 2020-01-27 at 09:48 -0600, Rob Herring wrote:
> > > On Mon, Jan 27, 2020 at 11:10:14AM +1300, Logan Shaw wrote:
> > > > Added a new file documenting the adt7475 devicetree and added
> > > > the
> > > > four
> > > > new properties to it.
> > > > 
> > > > Signed-off-by: Logan Shaw <logan.shaw@alliedtelesis.co.nz>
> > > > ---
> > > > +  bypass-attenuator-in0:
> > > 
> > > Needs a vendor prefix and a type ref.
> > 
> > Adi (Analog Devices) sold the ADT product line (amongst other
> > things)
> > to On Semiconductor. As changing the vendor of these chips (in
> > code)
> > would break backwards compatibility should we keep the vendor as
> > adi?
> 
> Yes. It should match what's used in the compatible string(s).
> 
> > To confirm, would this make the property "adi,adt7476,bypass-
> > attenuator-in0"?
> > 
> > So used in conjunction with patternProperties you would end up with
> > something like:
> > 
> > "adi,(adt7473|adt7475|adt7476|adt7490),bypass-attenuator-in[0134]"
> 
> No for the part #'s. Just add 'adi,'. Maybe you thought for type ref
> that's what I meant? A type ref is:
> 
> $ref: /schemas/types.yaml#/definitions/uint32

Yes, I was a little confused but now I am on the right track.

Re: [PATCH v6 2/2] dt-bindings: hwmon: (adt7475) Added missing adt7475 documentation

[Logan Shaw]

On Mon, 2020-01-27 at 09:48 -0600, Rob Herring wrote:
> On Mon, Jan 27, 2020 at 11:10:14AM +1300, Logan Shaw wrote:
> > Added a new file documenting the adt7475 devicetree and added the
> > four
> > new properties to it.
> > 
> > Signed-off-by: Logan Shaw <logan.shaw@alliedtelesis.co.nz>
> > ---
> > ---
> >  .../devicetree/bindings/hwmon/adt7475.yaml    | 95
> > +++++++++++++++++++
> >  1 file changed, 95 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/hwmon/adt7475.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> > b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> > new file mode 100644
> > index 000000000000..450da5e66e07
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> > @@ -0,0 +1,95 @@
> > +# SPDX-License-Identifier: GPL-2.0
> 
> Dual license new bindings please:
> 
> (GPL-2.0-only OR BSD-2-Clause)
> 
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/adt7475.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: ADT7475 hwmon sensor
> > +
> > +maintainers:
> > +  - Jean Delvare <jdelvare@suse.com>
> > +
> > +description: |
> > +  The ADT7473, ADT7475, ADT7476, and ADT7490 are thermal monitors
> > and multiple
> > +  PWN fan controllers.
> > +
> > +  They support monitoring and controlling up to four fans (the
> > ADT7490 can only
> > +  control up to three). They support reading a single on chip
> > temperature
> > +  sensor and two off chip temperature sensors (the ADT7490
> > additionally
> > +  supports measuring up to three current external temperature
> > sensors with
> > +  series resistance cancellation (SRC)).
> > +
> > +  Datasheets:
> > +  https://www.onsemi.com/pub/Collateral/ADT7473-D.PDF
> > +  https://www.onsemi.com/pub/Collateral/ADT7475-D.PDF
> > +  https://www.onsemi.com/pub/Collateral/ADT7476-D.PDF
> > +  https://www.onsemi.com/pub/Collateral/ADT7490-D.PDF
> > +
> > +  Description taken from omsemiconductors specification sheets,
> > with minor
> 
> omsemi?
>  ^
> 
> > +  rephrasing.
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - adi,adt7473
> > +      - adi,adt7475
> > +      - adi,adt7476
> > +      - adi,adt7490
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  bypass-attenuator-in0:
> 
> Needs a vendor prefix and a type ref. 
> 
> > +    description: |
> > +      Configures bypassing the individual voltage input
> > +      attenuator, on in0. This is supported on the ADT7476 and
> > ADT7490.
> > +      If set to a non-zero integer the attenuator is bypassed, if
> > set to
> > +      zero the attenuator is not bypassed. If the property is
> > absent then
> > +      the config register is not modified.
> 
> Sounds like this could be boolean? If not, define a schema for what
> are 
> valid values.

By boolean are you referring to a dts property that is evaluated (using
function of_property_read_bool) as true/false, depending on its
presence? For example "regulator-always-on" in:
/Documentation/devicetree/bindings/regulator/regulator.yaml .

> 
> > +    maxItems: 1
> > +
> > +  bypass-attenuator-in1:
> > +    description: |
> > +      Configures bypassing the individual voltage input
> > +      attenuator, on in1. This is supported on the ADT7473,
> > ADT7475,
> > +      ADT7476 and ADT7490. If set to a non-zero integer the
> > attenuator
> > +      is bypassed, if set to zero the attenuator is not bypassed.
> > If the
> > +      property is absent then the config register is not modified.
> > +    maxItems: 1
> > +
> > +  bypass-attenuator-in3:
> > +    description: |
> > +      Configures bypassing the individual voltage input
> > +      attenuator, on in3. This is supported on the ADT7476 and
> > ADT7490.
> > +      If set to a non-zero integer the attenuator is bypassed, if
> > set to
> > +      zero the attenuator is not bypassed. If the property is
> > absent then
> > +      the config register is not modified.
> > +    maxItems: 1
> > +
> > +  bypass-attenuator-in4:
> 
> These 4 could be a single entry under patternProperties.
> 
> 
> > +    description: |
> > +      Configures bypassing the individual voltage input
> > +      attenuator, on in4. This is supported on the ADT7476 and
> > ADT7490.
> > +      If set to a non-zero integer the attenuator is bypassed, if
> > set to
> > +      zero the attenuator is not bypassed. If the property is
> > absent then
> > +      the config register is not modified.
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +examples:
> > +  - |
> > +    i2c {
> > +      #address-cells = <1>;
> > +      #size-cells = <0>;
> > +
> > +      hwmon@2e {
> > +        compatible = "adi,adt7476";
> > +        reg = <0x2e>;
> > +        bypass-attenuator-in0 = <1>;
> > +        bypass-attenuator-in1 = <0>;
> > +      };
> > +    };
> > +...
> > -- 
> > 2.25.0
> > 

Re: [PATCH v6 2/2] dt-bindings: hwmon: (adt7475) Added missing adt7475 documentation

[Logan Shaw]

Just a gentle reminder as this thread has been quiet for a while. Rob,
I would appreciate some feedback on my question from my previous email. 

To add a little more detail: I want to avoid simply checking for the
existence of a property in the DTS as that could break compatibility
with codebases implementing their own method of handling the attenuator
bypass.

On Thu, 2020-01-30 at 03:07 +0000, Logan Shaw wrote:
> On Mon, 2020-01-27 at 09:48 -0600, Rob Herring wrote:
> > On Mon, Jan 27, 2020 at 11:10:14AM +1300, Logan Shaw wrote:
> > > Added a new file documenting the adt7475 devicetree and added the
> > > four
> > > new properties to it.
> > > 
> > > Signed-off-by: Logan Shaw <logan.shaw@alliedtelesis.co.nz>
> > > ---
> > > ---
> > >  .../devicetree/bindings/hwmon/adt7475.yaml    | 95
> > > +++++++++++++++++++
> > >  1 file changed, 95 insertions(+)
> > >  create mode 100644
> > > Documentation/devicetree/bindings/hwmon/adt7475.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> > > b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> > > new file mode 100644
> > > index 000000000000..450da5e66e07
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/hwmon/adt7475.yaml
> > > @@ -0,0 +1,95 @@
> > > +# SPDX-License-Identifier: GPL-2.0
> > 
> > Dual license new bindings please:
> > 
> > (GPL-2.0-only OR BSD-2-Clause)
> > 
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/adt7475.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: ADT7475 hwmon sensor
> > > +
> > > +maintainers:
> > > +  - Jean Delvare <jdelvare@suse.com>
> > > +
> > > +description: |
> > > +  The ADT7473, ADT7475, ADT7476, and ADT7490 are thermal
> > > monitors
> > > and multiple
> > > +  PWN fan controllers.
> > > +
> > > +  They support monitoring and controlling up to four fans (the
> > > ADT7490 can only
> > > +  control up to three). They support reading a single on chip
> > > temperature
> > > +  sensor and two off chip temperature sensors (the ADT7490
> > > additionally
> > > +  supports measuring up to three current external temperature
> > > sensors with
> > > +  series resistance cancellation (SRC)).
> > > +
> > > +  Datasheets:
> > > +  https://www.onsemi.com/pub/Collateral/ADT7473-D.PDF
> > > +  https://www.onsemi.com/pub/Collateral/ADT7475-D.PDF
> > > +  https://www.onsemi.com/pub/Collateral/ADT7476-D.PDF
> > > +  https://www.onsemi.com/pub/Collateral/ADT7490-D.PDF
> > > +
> > > +  Description taken from omsemiconductors specification sheets,
> > > with minor
> > 
> > omsemi?
> >  ^
> > 
> > > +  rephrasing.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    enum:
> > > +      - adi,adt7473
> > > +      - adi,adt7475
> > > +      - adi,adt7476
> > > +      - adi,adt7490
> > > +
> > > +  reg:
> > > +    maxItems: 1
> > > +
> > > +  bypass-attenuator-in0:
> > 
> > Needs a vendor prefix and a type ref. 
> > 
> > > +    description: |
> > > +      Configures bypassing the individual voltage input
> > > +      attenuator, on in0. This is supported on the ADT7476 and
> > > ADT7490.
> > > +      If set to a non-zero integer the attenuator is bypassed,
> > > if
> > > set to
> > > +      zero the attenuator is not bypassed. If the property is
> > > absent then
> > > +      the config register is not modified.
> > 
> > Sounds like this could be boolean? If not, define a schema for what
> > are 
> > valid values.
> 
> By boolean are you referring to a dts property that is evaluated
> (using
> function of_property_read_bool) as true/false, depending on its
> presence? For example "regulator-always-on" in:
> /Documentation/devicetree/bindings/regulator/regulator.yaml .
> 
> > 
> > > +    maxItems: 1
> > > +
> > > +  bypass-attenuator-in1:
> > > +    description: |
> > > +      Configures bypassing the individual voltage input
> > > +      attenuator, on in1. This is supported on the ADT7473,
> > > ADT7475,
> > > +      ADT7476 and ADT7490. If set to a non-zero integer the
> > > attenuator
> > > +      is bypassed, if set to zero the attenuator is not
> > > bypassed.
> > > If the
> > > +      property is absent then the config register is not
> > > modified.
> > > +    maxItems: 1
> > > +
> > > +  bypass-attenuator-in3:
> > > +    description: |
> > > +      Configures bypassing the individual voltage input
> > > +      attenuator, on in3. This is supported on the ADT7476 and
> > > ADT7490.
> > > +      If set to a non-zero integer the attenuator is bypassed,
> > > if
> > > set to
> > > +      zero the attenuator is not bypassed. If the property is
> > > absent then
> > > +      the config register is not modified.
> > > +    maxItems: 1
> > > +
> > > +  bypass-attenuator-in4:
> > 
> > These 4 could be a single entry under patternProperties.
> > 
> > 
> > > +    description: |
> > > +      Configures bypassing the individual voltage input
> > > +      attenuator, on in4. This is supported on the ADT7476 and
> > > ADT7490.
> > > +      If set to a non-zero integer the attenuator is bypassed,
> > > if
> > > set to
> > > +      zero the attenuator is not bypassed. If the property is
> > > absent then
> > > +      the config register is not modified.
> > > +    maxItems: 1
> > > +
> > > +required:
> > > +  - compatible
> > > +  - reg
> > > +
> > > +examples:
> > > +  - |
> > > +    i2c {
> > > +      #address-cells = <1>;
> > > +      #size-cells = <0>;
> > > +
> > > +      hwmon@2e {
> > > +        compatible = "adi,adt7476";
> > > +        reg = <0x2e>;
> > > +        bypass-attenuator-in0 = <1>;
> > > +        bypass-attenuator-in1 = <0>;
> > > +      };
> > > +    };
> > > +...
> > > -- 
> > > 2.25.0
> > >