Skip to contentSkip to conversation

Please suggest proper format for DT properties.

DISCUSSION Constantine Shulyupin 4 participants 7 messages Sep 18, 2015 – Sep 22, 2015

lore
Share & tools
lore
#0 lore raw

Hi,

I am designing DT support for a hwmon chip.
It has some sensors, each of them can be:
- "disabled"
- "thermal diode"
- "thermistor"
- "voltage"

Four possible options for DT properties format.

Option 1: Separated property for each sensor.

Example nct7802 node:

nct7802 {
	compatible = "nuvoton,nct7802";
	reg = <0x2a>;
	nuvoton,sensor1-type = "thermistor";
	nuvoton,sensor2-type = "disabled";
	nuvoton,sensor3-type = "voltage";
};

Option 2: Array of strings for all sensors.

nct7802 {
	compatible = "nuvoton,nct7802";
	reg = <0x2a>;
	nuvoton,sensors-types = "thermistor", "disabled", "voltage";
};

Option 3: Sets of 4 cells.

Borrowed from marvell,reg-init and broadcom,c45-reg-init.

The first cell is the page address,
the second a register address within the page,
the third cell contains a mask to be ANDed with the existing register value, and the fourth cell is ORed with the result to yield the new register value. If the third cell has a value of zero, no read of the existing value is performed.

Example nct7802 node:

nct7802 {
	compatible = "nuvoton,nct7802";
	reg = <0x2a>;
	nct7802,reg-init =
		<0 0x21 0 0x01 > // START = 1
		<0 0x22 0x03 0x02>; // RTD1_MD = 2
};

Please suggest proper format for DT properties.

Thanks
Constantine

PS:
Datasheet: https://www.nuvoton.com/hq/products/cloud-computing/hardware-monitors/desktop-server-series/nct7802y/

#1 lore raw
Quoted 23 lines · Constantine Shulyupin

Hi,

I am designing DT support for a hwmon chip.
It has some sensors, each of them can be:
- "disabled"
- "thermal diode"
- "thermistor"
- "voltage"

Four possible options for DT properties format.

Option 1: Separated property for each sensor.

Example nct7802 node:

nct7802 {
        compatible = "nuvoton,nct7802";
        reg = <0x2a>;
        nuvoton,sensor1-type = "thermistor";
        nuvoton,sensor2-type = "disabled";
        nuvoton,sensor3-type = "voltage";
};
Quoted 7 lines

Option 2: Array of strings for all sensors.

nct7802 {
        compatible = "nuvoton,nct7802";
        reg = <0x2a>;
        nuvoton,sensors-types = "thermistor", "disabled", "voltage";
};

It seems you are just listing out all possible modes. Why do you need this in the DT at all? This can be inferred by the compatible string.

Quoted 11 lines

Option 3: Sets of 4 cells.

Borrowed from marvell,reg-init and broadcom,c45-reg-init.

The first cell is the page address,
the second a register address within the page,
the third cell contains a mask to be ANDed with the existing register value, and the fourth cell is ORed with the result to yield the new register value. If the third cell has a value of zero, no read of the existing value is performed.

I don't see how this relates to the first 2 options. The register you write selects the mode? In general, we don't want bindings of just random register writes.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#2 lore raw
Quoted 27 lines · Rob Herring
Quoted 25 lines · Constantine Shulyupin

Hi,

I am designing DT support for a hwmon chip.
It has some sensors, each of them can be:
- "disabled"
- "thermal diode"
- "thermistor"
- "voltage"

Four possible options for DT properties format.

Option 1: Separated property for each sensor.

Example nct7802 node:

nct7802 {
        compatible = "nuvoton,nct7802";
        reg = <0x2a>;
        nuvoton,sensor1-type = "thermistor";
        nuvoton,sensor2-type = "disabled";
        nuvoton,sensor3-type = "voltage";
};

Option 2: Array of strings for all sensors.

Quoted 8 lines
Quoted 5 lines
nct7802 {
        compatible = "nuvoton,nct7802";
        reg = <0x2a>;
        nuvoton,sensors-types = "thermistor", "disabled", "voltage";
};

It seems you are just listing out all possible modes. Why do you need this in the DT at all? This can be inferred by the compatible string.

There are tree sensor, each of one can be one of three type ("thermal diode", "thermistor", "voltage") or be disabled.
Also there are at least five platform depended registers, most of them are enable/disable.
You can find full datasheet here
https://www.nuvoton.com/hq/products/cloud-computing/hardware-monitors/desktop-server-series/nct7802y/

The question is how to describe configuration of this registers in dts. The second option is array of types for tree registers.

Quoted 11 lines
Quoted 10 lines

Option 3: Sets of 4 cells.

Borrowed from marvell,reg-init and broadcom,c45-reg-init.

The first cell is the page address,
the second a register address within the page,
the third cell contains a mask to be ANDed with the existing register value, and the fourth cell is ORed with the result to yield the new register value. If the third cell has a value of zero, no read of the existing value is performed.

Quoted 5 lines

I don't see how this relates to the first 2 options. The register you write selects the mode? In general, we don't want bindings of just random register writes.

Rob

Option 3 allows arbitrary registers configuration.

To see the full picture you can refer to datasheet mentioned above to function nct7802_init_chip via
http://lxr.free-electrons.com/source/drivers/hwmon/nct7802.c#L789
and proposed patch for option #2: https://lkml.org/lkml/2015/9/13/204

Signature · 2 lines
Thanks
Constantine
#3 lore raw
Quoted 44 lines · Constantine Shulyupin
Quoted 27 lines · Rob Herring
Quoted 25 lines · Constantine Shulyupin

Hi,

I am designing DT support for a hwmon chip.
It has some sensors, each of them can be:
- "disabled"
- "thermal diode"
- "thermistor"
- "voltage"

Four possible options for DT properties format.

Option 1: Separated property for each sensor.

Example nct7802 node:

nct7802 {
        compatible = "nuvoton,nct7802";
        reg = <0x2a>;
        nuvoton,sensor1-type = "thermistor";
        nuvoton,sensor2-type = "disabled";
        nuvoton,sensor3-type = "voltage";
};

Option 2: Array of strings for all sensors.

Quoted 8 lines
Quoted 5 lines
nct7802 {
        compatible = "nuvoton,nct7802";
        reg = <0x2a>;
        nuvoton,sensors-types = "thermistor", "disabled", "voltage";
};

It seems you are just listing out all possible modes. Why do you need this in the DT at all? This can be inferred by the compatible string.

There are tree sensor, each of one can be one of three type ("thermal diode", "thermistor", "voltage") or be disabled.
Also there are at least five platform depended registers, most of them are enable/disable.
You can find full datasheet here
https://www.nuvoton.com/hq/products/cloud-computing/hardware-monitors/desktop-server-series/nct7802y/

Quoted 2 lines

The question is how to describe configuration of this registers in dts. The second option is array of types for tree registers.

Okay, got it. I would go with option 2 in that case. Perhaps "mode" instead of "type" though.

Rob
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#4 lore raw
In reply to Constantine Shulyupin
Quoted 43 lines · On Saturday 19 September 2015 01:36:43 Constantine Shulyupin wrote:

I am designing DT support for a hwmon chip.
It has some sensors, each of them can be:
- "disabled"
- "thermal diode"
- "thermistor"
- "voltage"

Four possible options for DT properties format.

Option 1: Separated property for each sensor.

Example nct7802 node:

nct7802 {
        compatible = "nuvoton,nct7802";
        reg = <0x2a>;
        nuvoton,sensor1-type = "thermistor";
        nuvoton,sensor2-type = "disabled";
        nuvoton,sensor3-type = "voltage";
};

Option 2: Array of strings for all sensors.

nct7802 {
        compatible = "nuvoton,nct7802";
        reg = <0x2a>;
        nuvoton,sensors-types = "thermistor", "disabled", "voltage";
};

Option 3: Sets of 4 cells.

Borrowed from marvell,reg-init and broadcom,c45-reg-init.

The first cell is the page address,
the second a register address within the page,
the third cell contains a mask to be ANDed with the existing register value, and the fourth cell is ORed with the result to yield the new register value. If the third cell has a value of zero, no read of the existing value is performed.

Example nct7802 node:

Quoted 8 lines
nct7802 {
        compatible = "nuvoton,nct7802";
        reg = <0x2a>;
        nct7802,reg-init =
                <0 0x21 0 0x01 > // START = 1
                <0 0x22 0x03 0x02>; // RTD1_MD = 2
};

I would strongly prefer Option 1 or 2 over option 3. Between 1 and 2, I'd probably go for 1. Another option might be to have a subnode per sensor:

nct7802@2a {
        compatible = "nuvoton,nct7802";
        reg = <0x2a>;
	#address-cells=<1>;
	#size-cells=<0>;

	sensor@1 {
		compatible = "nuvoton,nct7802-thermistor";
		further-properties;
	};
	sensor@3 {
		compatible = "nuvoton,nct7802-voltage";
		for-example-range-mv = <0 5000>;
	};
};

In either case, I'd say that disabled sensors should not need to be listed.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html

#5 lore raw
Quoted 57 lines · Arnd Bergmann
Quoted 43 lines · On Saturday 19 September 2015 01:36:43 Constantine Shulyupin wrote:

I am designing DT support for a hwmon chip.
It has some sensors, each of them can be:
- "disabled"
- "thermal diode"
- "thermistor"
- "voltage"

Four possible options for DT properties format.

Option 1: Separated property for each sensor.

Example nct7802 node:

nct7802 {
         compatible = "nuvoton,nct7802";
         reg = <0x2a>;
         nuvoton,sensor1-type = "thermistor";
         nuvoton,sensor2-type = "disabled";
         nuvoton,sensor3-type = "voltage";
};

Option 2: Array of strings for all sensors.

nct7802 {
         compatible = "nuvoton,nct7802";
         reg = <0x2a>;
         nuvoton,sensors-types = "thermistor", "disabled", "voltage";
};

Option 3: Sets of 4 cells.

Borrowed from marvell,reg-init and broadcom,c45-reg-init.

The first cell is the page address,
the second a register address within the page,
the third cell contains a mask to be ANDed with the existing register value, and the fourth cell is ORed with the result to yield the new register value. If the third cell has a value of zero, no read of the existing value is performed.

Example nct7802 node:

Quoted 8 lines
nct7802 {
         compatible = "nuvoton,nct7802";
         reg = <0x2a>;
         nct7802,reg-init =
                 <0 0x21 0 0x01 > // START = 1
                 <0 0x22 0x03 0x02>; // RTD1_MD = 2
};

I would strongly prefer Option 1 or 2 over option 3. Between 1 and 2, I'd probably go for 1. Another option might be to have a subnode per sensor:

Quoted 16 lines
nct7802@2a {
         compatible = "nuvoton,nct7802";
         reg = <0x2a>;
	#address-cells=<1>;
	#size-cells=<0>;

	sensor@1 {
		compatible = "nuvoton,nct7802-thermistor";
		further-properties;
	};
	sensor@3 {
		compatible = "nuvoton,nct7802-voltage";
		for-example-range-mv = <0 5000>;
	};
};

I personally would prefer this approach. It would also make it easier to add more properties. Wonder what is more appropriate, though - a compatible property or something like the following ?

		sensor-type = "xxx";

I don't have a preference, just asking.

Also, would the index be derived from "@1", or should there be a reg property ?

Quoted 3 lines

In either case, I'd say that disabled sensors should not need to be listed.

Agreed.

Thanks,
Guenter

#6 lore raw
Quoted 20 lines · On Tuesday 22 September 2015 08:08:25 Guenter Roeck wrote:
Quoted 4 lines

I would strongly prefer Option 1 or 2 over option 3. Between 1 and 2, I'd probably go for 1. Another option might be to have a subnode per sensor:

Quoted 16 lines
nct7802@2a {
         compatible = "nuvoton,nct7802";
         reg = <0x2a>;
      #address-cells=<1>;
      #size-cells=<0>;

      sensor@1 {
              compatible = "nuvoton,nct7802-thermistor";
              further-properties;
      };
      sensor@3 {
              compatible = "nuvoton,nct7802-voltage";
              for-example-range-mv = <0 5000>;
      };
};
Quoted 6 lines

I personally would prefer this approach. It would also make it easier to add more properties. Wonder what is more appropriate, though - a compatible property or something like the following ?

                sensor-type = "xxx";

I don't have a preference, just asking.

I'm not sure here, either way would work, and we are not particularly consistent in this regard. Maybe someone else has a stronger preference.

Quoted 1 line

Also, would the index be derived from "@1", or should there be a reg property ?

There needs to be a 'reg' property. Sorry for missing that above.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html