Change in coreboot[master]: drivers/i2c/nct7802y: configure local temp sensor [WIP]

List overview All Threads
Download

newer

older

Change in coreboot[master]:...

Maxim Polyakov (Code Review)

22 Mar 2020 22 Mar '20

10:02 p.m.

Maxim Polyakov has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39766 )

Change subject: drivers/i2c/nct7802y: configure local temp sensor [WIP] ......................................................................

drivers/i2c/nct7802y: configure local temp sensor [WIP]

According to the documentation[1], the Mode Selection Register sets the operation mode of the local temperature sensors. The patch allows to do this from a tree of devices for a board that uses uses this HWM.

[1] page 30, section 7.2.32, Nuvoton Hardware Monitoring IC NCT7802Y with PECI 3.0 interface, datasheet, revision 1.2, february 2012

Change-Id: I28cc4e5cae76cf0bcdad26a50ee6cd43a201d31e --- M src/drivers/i2c/nct7802y/chip.h M src/drivers/i2c/nct7802y/nct7802y.h M src/drivers/i2c/nct7802y/nct7802y_fan.c 3 files changed, 33 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/39766/1

diff --git a/src/drivers/i2c/nct7802y/chip.h b/src/drivers/i2c/nct7802y/chip.h index adff0f5..693cc41 100644 --- a/src/drivers/i2c/nct7802y/chip.h +++ b/src/drivers/i2c/nct7802y/chip.h @@ -19,6 +19,13 @@ #define NCT7802Y_PECI_CNT 2 #define NCT7802Y_FAN_CNT 3

+enum nct7802y_sensor_mode_type { + RTD_CLOSED = 0, + RTD_CURRENT_MODE, + RTD_THERMISTOR_MODE, + RTD_VOLTAGE_MODE, +}; + enum nct7802y_peci_mode { PECI_DISABLED = 0, PECI_DOMAIN_0, @@ -64,6 +71,14 @@ TEMP_SOURCE_PROGRAMMABLE_1, };

+/* Local temperature sensor configuration */ +struct nct7802y_sensor_mode_selection { + bool local_temp_sensor_enable; + enum nct7802y_sensor_mode_type rtd3_md; + enum nct7802y_sensor_mode_type rtd2_md; + enum nct7802y_sensor_mode_type rtd1_md; +}; + struct nct7802y_fan_smartconfig { enum nct7802y_fan_smartmode mode; enum nct7802y_fan_speed speed; @@ -85,6 +100,7 @@

/* Implements only those parts currently used by coreboot mainboards. */ struct drivers_i2c_nct7802y_config { + struct nct7802y_sensor_mode_selection sensor_mode; struct nct7802y_peci_config peci[NCT7802Y_PECI_CNT]; struct nct7802y_fan_config fan[NCT7802Y_FAN_CNT]; enum nct7802y_fan_pecierror on_pecierror; diff --git a/src/drivers/i2c/nct7802y/nct7802y.h b/src/drivers/i2c/nct7802y/nct7802y.h index 9f3aaef..a105fea 100644 --- a/src/drivers/i2c/nct7802y/nct7802y.h +++ b/src/drivers/i2c/nct7802y/nct7802y.h @@ -23,6 +23,15 @@

/* Bank 0 */

+#define MODE_SELECTION 0x22 +#define MODE_SELECTION_EN_LTD(x) (x << 6) +#define MODE_SELECTION_RTD3_MD(x) (x << 4) +#define MODE_SELECTION_RTD2_MD(x) (x << 2) +#define MODE_SELECTION_RTD1_MD(x) (x) + +#define PECI_ENABLE 0x23 +#define PECI_ENABLE_AGENTx(x) (1 << (x)) + #define PECI_ENABLE 0x23 #define PECI_ENABLE_AGENTx(x) (1 << (x))

diff --git a/src/drivers/i2c/nct7802y/nct7802y_fan.c b/src/drivers/i2c/nct7802y/nct7802y_fan.c index d7cfb90..1193bb9 100644 --- a/src/drivers/i2c/nct7802y/nct7802y_fan.c +++ b/src/drivers/i2c/nct7802y/nct7802y_fan.c @@ -89,6 +89,14 @@ init_fan(dev, &config->fan[i], i); }

+ if (config->sensor_mode.local_temp_sensor_enable) { + nct7802y_write(dev, MODE_SELECTION, + MODE_SELECTION_EN_LTD(config->sensor_mode.local_temp_sensor_enable) | + MODE_SELECTION_RTD3_MD(config->sensor_mode.rtd3_md) | + MODE_SELECTION_RTD2_MD(config->sensor_mode.rtd2_md) | + MODE_SELECTION_RTD1_MD(config->sensor_mode.rtd1_md)); + } + switch (config->on_pecierror) { case PECI_ERROR_KEEP: set = CLOSE_LOOP_FAN_PECI_ERR_CURR;

Attachments:

attachment.htm (text/html — 7.0 KB)

Show replies by date

build bot (Jenkins) (Code Review)

22 Mar 22 Mar

10:03 p.m.

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39766 )

Change subject: drivers/i2c/nct7802y: configure local temp sensor [WIP] ......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39766/1/src/drivers/i2c/nct7802y/nc... File src/drivers/i2c/nct7802y/nct7802y_fan.c:

https://review.coreboot.org/c/coreboot/+/39766/1/src/drivers/i2c/nct7802y/nc... PS1, Line 94: MODE_SELECTION_EN_LTD(config->sensor_mode.local_temp_sensor_enable) | line over 96 characters

Maxim Polyakov (Code Review)

10:18 p.m.

Hello build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/39766

to look at the new patch set (#2).

Change subject: drivers/i2c/nct7802y: configure local temp sensor [WIP] ......................................................................

drivers/i2c/nct7802y: configure local temp sensor [WIP]

[1] page 30, section 7.2.32, Nuvoton Hardware Monitoring IC NCT7802Y with PECI 3.0 interface, datasheet, revision 1.2, february 2012

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/39766/2

build bot (Jenkins) (Code Review)

10:19 p.m.

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39766 )

Change subject: drivers/i2c/nct7802y: configure local temp sensor [WIP] ......................................................................

Patch Set 2:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39766/2/src/drivers/i2c/nct7802y/nc... File src/drivers/i2c/nct7802y/nct7802y_fan.c:

https://review.coreboot.org/c/coreboot/+/39766/2/src/drivers/i2c/nct7802y/nc... PS2, Line 94: MODE_SELECTION_EN_LTD(config->sensor_mode.local_temp_sensor_enable) | line over 96 characters

build bot (Jenkins) (Code Review)

10:21 p.m.

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39766 )

Change subject: drivers/i2c/nct7802y: configure local temp sensor [WIP] ......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39766/3/src/drivers/i2c/nct7802y/nc... File src/drivers/i2c/nct7802y/nct7802y_fan.c:

https://review.coreboot.org/c/coreboot/+/39766/3/src/drivers/i2c/nct7802y/nc... PS3, Line 94: MODE_SELECTION_EN_LTD(config->sensor_mode.local_temp_sensor_enable) | line over 96 characters

build bot (Jenkins) (Code Review)

11:18 p.m.

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39766 )

Change subject: drivers/i2c/nct7802y: configure local temp sensor [WIP] ......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39766/4/src/drivers/i2c/nct7802y/nc... File src/drivers/i2c/nct7802y/nct7802y_fan.c:

https://review.coreboot.org/c/coreboot/+/39766/4/src/drivers/i2c/nct7802y/nc... PS4, Line 94: MODE_SELECTION_EN_LTD(config->sensor_mode.local_temp_sensor_enable) | line over 96 characters

build bot (Jenkins) (Code Review)

11:23 p.m.

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39766 )

Change subject: drivers/i2c/nct7802y: configure local temp sensor [WIP] ......................................................................

Patch Set 5:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39766/5/src/drivers/i2c/nct7802y/nc... File src/drivers/i2c/nct7802y/nct7802y_fan.c:

https://review.coreboot.org/c/coreboot/+/39766/5/src/drivers/i2c/nct7802y/nc... PS5, Line 94: MODE_SELECTION_EN_LTD(config->sensor_mode.local_temp_sensor_enable) | line over 96 characters

build bot (Jenkins) (Code Review)

23 Mar 23 Mar

8:48 a.m.

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39766 )

Change subject: drivers/i2c/nct7802y: configure local temp sensor [WIP] ......................................................................

Patch Set 6:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39766/6/src/drivers/i2c/nct7802y/nc... File src/drivers/i2c/nct7802y/nct7802y_fan.c:

https://review.coreboot.org/c/coreboot/+/39766/6/src/drivers/i2c/nct7802y/nc... PS6, Line 94: MODE_SELECTION_EN_LTD(config->sensor_mode.local_temp_sensor_enable) | line over 96 characters

build bot (Jenkins) (Code Review)

9:28 a.m.

build bot (Jenkins) has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39766 )

Change subject: drivers/i2c/nct7802y: configure local temp sensor [WIP] ......................................................................

Patch Set 7:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39766/7/src/drivers/i2c/nct7802y/nc... File src/drivers/i2c/nct7802y/nct7802y_fan.c:

https://review.coreboot.org/c/coreboot/+/39766/7/src/drivers/i2c/nct7802y/nc... PS7, Line 94: MODE_SELECTION_EN_LTD(config->sensor_mode.local_temp_sensor_enable) | line over 96 characters

Maxim Polyakov (Code Review)

25 Mar 25 Mar

9:04 p.m.

New subject: Change in coreboot[master]: drivers/i2c/nct7802y: configure local temp sensor

Hello build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/39766

to look at the new patch set (#8).

Change subject: drivers/i2c/nct7802y: configure local temp sensor ......................................................................

drivers/i2c/nct7802y: configure local temp sensor

[1] page 30, section 7.2.32, Nuvoton Hardware Monitoring IC NCT7802Y with PECI 3.0 interface, datasheet, revision 1.2, february 2012

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/39766/8

Maxim Polyakov (Code Review)

10:30 p.m.

New subject: Change in coreboot[master]: drivers/i2c/nct7802y: configure local temp sensor

Hello build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/39766

to look at the new patch set (#9).

Change subject: drivers/i2c/nct7802y: configure local temp sensor ......................................................................

drivers/i2c/nct7802y: configure local temp sensor

[1] page 30, section 7.2.32, Nuvoton Hardware Monitoring IC NCT7802Y with PECI 3.0 interface, datasheet, revision 1.2, february 2012

Change-Id: I28cc4e5cae76cf0bcdad26a50ee6cd43a201d31e Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com --- M src/drivers/i2c/nct7802y/chip.h M src/drivers/i2c/nct7802y/nct7802y.h M src/drivers/i2c/nct7802y/nct7802y_fan.c 3 files changed, 29 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/39766/9

Maxim Polyakov (Code Review)

27 Aug 27 Aug

8:26 p.m.

New subject: Change in coreboot[master]: drivers/i2c/nct7802y: configure local temp sensor

Hello build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/39766

to look at the new patch set (#25).

Change subject: drivers/i2c/nct7802y: configure local temp sensor ......................................................................

drivers/i2c/nct7802y: configure local temp sensor

According to the documentation [1], the Mode Selection Register sets the operation mode of the local temperature sensors. It is necessary for some Intel processors (Apollo Lake SoC) that do not support PECI and the CPU temperature is taken from the thermistor (NTC).

This patch allows to configure local sensor mode from a tree of devices for a board that uses this HWM.

TEST = After loading the nct7802 module on the Kontron mAL-10 [2] with Linux OS, we can see configuration of the HWM with one sensor in the thermistor mode:

user@user-apl:~$ sensors coretemp-isa-0000 Adapter: ISA adapter Package id 0: +41.0°C (high = +110.0°C, crit = +110.0°C) Core 0: +40.0°C (high = +110.0°C, crit = +110.0°C) Core 1: +40.0°C (high = +110.0°C, crit = +110.0°C) Core 2: +41.0°C (high = +110.0°C, crit = +110.0°C) Core 3: +41.0°C (high = +110.0°C, crit = +110.0°C)

nct7802-i2c-0-2e Adapter: SMBus CMI adapter cmi in0: +3.35 V (min = +0.00 V, max = +4.09 V) in1: +1.92 V in3: +1.21 V (min = +0.00 V, max = +2.05 V) in4: +1.68 V (min = +0.00 V, max = +2.05 V) fan1: 0 RPM (min = 0 RPM) fan2: 868 RPM (min = 0 RPM) fan3: 0 RPM (min = 0 RPM) temp1: +42.5°C (low = +0.0°C, high = +85.0°C) (crit = +100.0°C) sensor = thermistor temp4: +44.0°C (low = +0.0°C, high = +85.0°C) (crit = +100.0°C) temp6: +0.0°C

[1] page 30, section 7.2.32, Nuvoton Hardware Monitoring IC NCT7802Y with PECI 3.0 interface, datasheet, revision 1.2, february 2012 [2] https://review.coreboot.org/c/coreboot/+/39133

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/39766/25

Mario Scheithauer (Code Review)

15 Sep 15 Sep

1:07 p.m.

New subject: Change in coreboot[master]: drivers/i2c/nct7802y: configure local temp sensor

Mario Scheithauer has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39766 )

Change subject: drivers/i2c/nct7802y: configure local temp sensor ......................................................................

Patch Set 29: Code-Review+1

-- To view, visit https://review.coreboot.org/c/coreboot/+/39766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I28cc4e5cae76cf0bcdad26a50ee6cd43a201d31e Gerrit-Change-Number: 39766 Gerrit-PatchSet: 29 Gerrit-Owner: Maxim Polyakov max.senia.poliak@gmail.com Gerrit-Reviewer: Felix Singer felixsinger@posteo.net Gerrit-Reviewer: Andrey Petrov andrey.petrov@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Lance Zhao Gerrit-Reviewer: Mario Scheithauer mario.scheithauer@siemens.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Werner Zeh werner.zeh@siemens.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Tue, 15 Sep 2020 11:07:01 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Andrey Petrov (Code Review)

22 Sep 22 Sep

5:23 p.m.

New subject: Change in coreboot[master]: drivers/i2c/nct7802y: configure local temp sensor

Andrey Petrov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39766 )

Change subject: drivers/i2c/nct7802y: configure local temp sensor ......................................................................

Patch Set 29: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/39766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I28cc4e5cae76cf0bcdad26a50ee6cd43a201d31e Gerrit-Change-Number: 39766 Gerrit-PatchSet: 29 Gerrit-Owner: Maxim Polyakov max.senia.poliak@gmail.com Gerrit-Reviewer: Felix Singer felixsinger@posteo.net Gerrit-Reviewer: Andrey Petrov andrey.petrov@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Lance Zhao Gerrit-Reviewer: Mario Scheithauer mario.scheithauer@siemens.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Werner Zeh werner.zeh@siemens.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Tue, 22 Sep 2020 15:23:09 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Nico Huber (Code Review)

27 Sep 27 Sep

8 p.m.

New subject: Change in coreboot[master]: drivers/i2c/nct7802y: configure local temp sensor

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39766 )

Change subject: drivers/i2c/nct7802y: configure local temp sensor ......................................................................

Patch Set 30: Code-Review+2

(2 comments)

https://review.coreboot.org/c/coreboot/+/39766/30/src/drivers/i2c/nct7802y/c... File src/drivers/i2c/nct7802y/chip.h:

https://review.coreboot.org/c/coreboot/+/39766/30/src/drivers/i2c/nct7802y/c... PS30, Line 68: enum nct7802y_sensor_mode rtd1; Nit, we use arrays, usually.

https://review.coreboot.org/c/coreboot/+/39766/30/src/drivers/i2c/nct7802y/n... File src/drivers/i2c/nct7802y/nct7802y.h:

https://review.coreboot.org/c/coreboot/+/39766/30/src/drivers/i2c/nct7802y/n... PS30, Line 15: #define MODE_SELECTION_EN_LTD(x) (x << 6) Nit, if it's a single bit, it doesn't need a parameter.

-- To view, visit https://review.coreboot.org/c/coreboot/+/39766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I28cc4e5cae76cf0bcdad26a50ee6cd43a201d31e Gerrit-Change-Number: 39766 Gerrit-PatchSet: 30 Gerrit-Owner: Maxim Polyakov max.senia.poliak@gmail.com Gerrit-Reviewer: Felix Singer felixsinger@posteo.net Gerrit-Reviewer: Andrey Petrov andrey.petrov@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Lance Zhao Gerrit-Reviewer: Mario Scheithauer mario.scheithauer@siemens.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Werner Zeh werner.zeh@siemens.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Sun, 27 Sep 2020 18:00:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Angel Pons (Code Review)

10:34 p.m.

New subject: Change in coreboot[master]: drivers/i2c/nct7802y: configure local temp sensor

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39766 )

Change subject: drivers/i2c/nct7802y: configure local temp sensor ......................................................................

Patch Set 30:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39766/30/src/drivers/i2c/nct7802y/n... File src/drivers/i2c/nct7802y/nct7802y.h:

https://review.coreboot.org/c/coreboot/+/39766/30/src/drivers/i2c/nct7802y/n... PS30, Line 17: #define MODE_SELECTION_RTD2(x) (x << 2) Please guard macro parameters with braces as done below.

-- To view, visit https://review.coreboot.org/c/coreboot/+/39766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I28cc4e5cae76cf0bcdad26a50ee6cd43a201d31e Gerrit-Change-Number: 39766 Gerrit-PatchSet: 30 Gerrit-Owner: Maxim Polyakov max.senia.poliak@gmail.com Gerrit-Reviewer: Felix Singer felixsinger@posteo.net Gerrit-Reviewer: Andrey Petrov andrey.petrov@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Lance Zhao Gerrit-Reviewer: Mario Scheithauer mario.scheithauer@siemens.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Werner Zeh werner.zeh@siemens.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Sun, 27 Sep 2020 20:34:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Maxim Polyakov (Code Review)

28 Sep 28 Sep

11:29 a.m.

New subject: Change in coreboot[master]: drivers/i2c/nct7802y: Configure remote diodes and local sensor

Hello Lance Zhao, Felix Singer, build bot (Jenkins), Nico Huber, Mario Scheithauer, Angel Pons, Andrey Petrov, Werner Zeh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/39766

to look at the new patch set (#31).

Change subject: drivers/i2c/nct7802y: Configure remote diodes and local sensor ......................................................................

drivers/i2c/nct7802y: Configure remote diodes and local sensor

The patch allows to configure sensors with a remote diode connected and a on-chip local temperature sensor from the devicetree for the board that uses this HWM. According to the documentation [1], this is done by setting the corresponding bits in the Mode Selection Register (22h). It is necessary for some Intel processors (Apollo Lake SoC) that do not support PECI and the CPU temperature is taken from the thermistor.

TEST = After loading the nct7802 module on the Kontron mAL-10 [2] with Linux OS, we can see configuration of the HWM with one sensor in the thermistor mode:

[1] page 30, section 7.2.32, Nuvoton Hardware Monitoring IC NCT7802Y with PECI 3.0 interface, datasheet, revision 1.2, february 2012 [2] https://review.coreboot.org/c/coreboot/+/39133

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/39766/31

-- To view, visit https://review.coreboot.org/c/coreboot/+/39766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I28cc4e5cae76cf0bcdad26a50ee6cd43a201d31e Gerrit-Change-Number: 39766 Gerrit-PatchSet: 31 Gerrit-Owner: Maxim Polyakov max.senia.poliak@gmail.com Gerrit-Reviewer: Felix Singer felixsinger@posteo.net Gerrit-Reviewer: Andrey Petrov andrey.petrov@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Lance Zhao Gerrit-Reviewer: Mario Scheithauer mario.scheithauer@siemens.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Werner Zeh werner.zeh@siemens.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: newpatchset

Maxim Polyakov (Code Review)

12:01 p.m.

New subject: Change in coreboot[master]: drivers/i2c/nct7802y: Configure remote diodes and local sensor

Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39766 )

Change subject: drivers/i2c/nct7802y: Configure remote diodes and local sensor ......................................................................

Patch Set 31:

(3 comments)

Thanks for the review!

https://review.coreboot.org/c/coreboot/+/39766/30/src/drivers/i2c/nct7802y/c... File src/drivers/i2c/nct7802y/chip.h:

https://review.coreboot.org/c/coreboot/+/39766/30/src/drivers/i2c/nct7802y/c... PS30, Line 68: enum nct7802y_sensor_mode rtd1;

...

Nit, we use arrays, usually.

Hi Niсo! Thanks for your comments. I reworked this patch. please review again

https://review.coreboot.org/c/coreboot/+/39766/30/src/drivers/i2c/nct7802y/n... File src/drivers/i2c/nct7802y/nct7802y.h:

https://review.coreboot.org/c/coreboot/+/39766/30/src/drivers/i2c/nct7802y/n... PS30, Line 15: #define MODE_SELECTION_EN_LTD(x) (x << 6)

...

Nit, if it's a single bit, it doesn't need a parameter.

Done

https://review.coreboot.org/c/coreboot/+/39766/30/src/drivers/i2c/nct7802y/n... PS30, Line 17: #define MODE_SELECTION_RTD2(x) (x << 2)

...

Please guard macro parameters with braces as done below.

These macros have been removed

-- To view, visit https://review.coreboot.org/c/coreboot/+/39766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I28cc4e5cae76cf0bcdad26a50ee6cd43a201d31e Gerrit-Change-Number: 39766 Gerrit-PatchSet: 31 Gerrit-Owner: Maxim Polyakov max.senia.poliak@gmail.com Gerrit-Reviewer: Felix Singer felixsinger@posteo.net Gerrit-Reviewer: Andrey Petrov andrey.petrov@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Lance Zhao Gerrit-Reviewer: Mario Scheithauer mario.scheithauer@siemens.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Werner Zeh werner.zeh@siemens.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Mon, 28 Sep 2020 10:01:06 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber nico.h@gmx.de Comment-In-Reply-To: Angel Pons th3fanbus@gmail.com Gerrit-MessageType: comment

Angel Pons (Code Review)

12:09 p.m.

New subject: Change in coreboot[master]: drivers/i2c/nct7802y: Configure remote diodes and local sensor

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39766 )

Change subject: drivers/i2c/nct7802y: Configure remote diodes and local sensor ......................................................................

Patch Set 31:

(2 comments)

https://review.coreboot.org/c/coreboot/+/39766/31/src/drivers/i2c/nct7802y/n... File src/drivers/i2c/nct7802y/nct7802y_fan.c:

https://review.coreboot.org/c/coreboot/+/39766/31/src/drivers/i2c/nct7802y/n... PS31, Line 81: value = 0 Initialize this when declaring the variable, please

https://review.coreboot.org/c/coreboot/+/39766/31/src/drivers/i2c/nct7802y/n... PS31, Line 82: (i << 1) i * 2?

-- To view, visit https://review.coreboot.org/c/coreboot/+/39766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I28cc4e5cae76cf0bcdad26a50ee6cd43a201d31e Gerrit-Change-Number: 39766 Gerrit-PatchSet: 31 Gerrit-Owner: Maxim Polyakov max.senia.poliak@gmail.com Gerrit-Reviewer: Felix Singer felixsinger@posteo.net Gerrit-Reviewer: Andrey Petrov andrey.petrov@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Lance Zhao Gerrit-Reviewer: Mario Scheithauer mario.scheithauer@siemens.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Werner Zeh werner.zeh@siemens.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Mon, 28 Sep 2020 10:09:10 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

Nico Huber (Code Review)

12:17 p.m.

New subject: Change in coreboot[master]: drivers/i2c/nct7802y: Configure remote diodes and local sensor

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39766 )

Change subject: drivers/i2c/nct7802y: Configure remote diodes and local sensor ......................................................................

Patch Set 31: Code-Review+1

(1 comment)

Thanks for the update. I only now realized that you put the code amongst the fan init. Any particular reason? I don't think it needs its own file, but would prefer it in `nct7802y.c`, maybe just a small function nct7802y_init_sensors()? But I don't feel strong about it.

https://review.coreboot.org/c/coreboot/+/39766/31/src/drivers/i2c/nct7802y/n... File src/drivers/i2c/nct7802y/nct7802y_fan.c:

https://review.coreboot.org/c/coreboot/+/39766/31/src/drivers/i2c/nct7802y/n... PS31, Line 82: i << 1 Please just write `i * 2`. This is not very readable. Also wouldn't need parentheses.

Could also be a macro, e.g.

#define MODE_SELECTION_RTDx(x, val) ((val) << (x) * 2)

-- To view, visit https://review.coreboot.org/c/coreboot/+/39766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I28cc4e5cae76cf0bcdad26a50ee6cd43a201d31e Gerrit-Change-Number: 39766 Gerrit-PatchSet: 31 Gerrit-Owner: Maxim Polyakov max.senia.poliak@gmail.com Gerrit-Reviewer: Felix Singer felixsinger@posteo.net Gerrit-Reviewer: Andrey Petrov andrey.petrov@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Lance Zhao Gerrit-Reviewer: Mario Scheithauer mario.scheithauer@siemens.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Werner Zeh werner.zeh@siemens.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Mon, 28 Sep 2020 10:17:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Nico Huber (Code Review)

12:19 p.m.

New subject: Change in coreboot[master]: drivers/i2c/nct7802y: Configure remote diodes and local sensor

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39766 )

Change subject: drivers/i2c/nct7802y: Configure remote diodes and local sensor ......................................................................

Patch Set 31:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39766/31/src/drivers/i2c/nct7802y/n... File src/drivers/i2c/nct7802y/nct7802y_fan.c:

https://review.coreboot.org/c/coreboot/+/39766/31/src/drivers/i2c/nct7802y/n... PS31, Line 81: value = 0

...

Initialize this when declaring the variable, please

Hmmm, the variable is used for multiple things, would be odd to not have the initialization local to this use. Could be a separate line, though?

-- To view, visit https://review.coreboot.org/c/coreboot/+/39766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I28cc4e5cae76cf0bcdad26a50ee6cd43a201d31e Gerrit-Change-Number: 39766 Gerrit-PatchSet: 31 Gerrit-Owner: Maxim Polyakov max.senia.poliak@gmail.com Gerrit-Reviewer: Felix Singer felixsinger@posteo.net Gerrit-Reviewer: Andrey Petrov andrey.petrov@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Lance Zhao Gerrit-Reviewer: Mario Scheithauer mario.scheithauer@siemens.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Werner Zeh werner.zeh@siemens.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Mon, 28 Sep 2020 10:19:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Angel Pons th3fanbus@gmail.com Gerrit-MessageType: comment

Angel Pons (Code Review)

12:27 p.m.

New subject: Change in coreboot[master]: drivers/i2c/nct7802y: Configure remote diodes and local sensor

Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39766 )

Change subject: drivers/i2c/nct7802y: Configure remote diodes and local sensor ......................................................................

Patch Set 31:

(1 comment)

https://review.coreboot.org/c/coreboot/+/39766/31/src/drivers/i2c/nct7802y/n... File src/drivers/i2c/nct7802y/nct7802y_fan.c:

https://review.coreboot.org/c/coreboot/+/39766/31/src/drivers/i2c/nct7802y/n... PS31, Line 81: value = 0

...

Hmmm, the variable is used for multiple things, would be odd to not have […]

Right, sounds good.

-- To view, visit https://review.coreboot.org/c/coreboot/+/39766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I28cc4e5cae76cf0bcdad26a50ee6cd43a201d31e Gerrit-Change-Number: 39766 Gerrit-PatchSet: 31 Gerrit-Owner: Maxim Polyakov max.senia.poliak@gmail.com Gerrit-Reviewer: Felix Singer felixsinger@posteo.net Gerrit-Reviewer: Andrey Petrov andrey.petrov@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Lance Zhao Gerrit-Reviewer: Mario Scheithauer mario.scheithauer@siemens.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Werner Zeh werner.zeh@siemens.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Mon, 28 Sep 2020 10:27:30 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber nico.h@gmx.de Comment-In-Reply-To: Angel Pons th3fanbus@gmail.com Gerrit-MessageType: comment

Maxim Polyakov (Code Review)

2:03 p.m.

New subject: Change in coreboot[master]: drivers/i2c/nct7802y: Configure remote diodes and local sensor

Hello Lance Zhao, Felix Singer, build bot (Jenkins), Nico Huber, Mario Scheithauer, Angel Pons, Andrey Petrov, Werner Zeh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/39766

to look at the new patch set (#32).

Change subject: drivers/i2c/nct7802y: Configure remote diodes and local sensor ......................................................................

drivers/i2c/nct7802y: Configure remote diodes and local sensor

TEST = After loading the nct7802 module on the Kontron mAL-10 [2] with Linux OS, we can see configuration of the HWM with one sensor in the thermistor mode:

[1] page 30, section 7.2.32, Nuvoton Hardware Monitoring IC NCT7802Y with PECI 3.0 interface, datasheet, revision 1.2, february 2012 [2] https://review.coreboot.org/c/coreboot/+/39133

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/66/39766/32

-- To view, visit https://review.coreboot.org/c/coreboot/+/39766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I28cc4e5cae76cf0bcdad26a50ee6cd43a201d31e Gerrit-Change-Number: 39766 Gerrit-PatchSet: 32 Gerrit-Owner: Maxim Polyakov max.senia.poliak@gmail.com Gerrit-Reviewer: Felix Singer felixsinger@posteo.net Gerrit-Reviewer: Andrey Petrov andrey.petrov@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Lance Zhao Gerrit-Reviewer: Mario Scheithauer mario.scheithauer@siemens.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Werner Zeh werner.zeh@siemens.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: newpatchset

Maxim Polyakov (Code Review)

2:15 p.m.

New subject: Change in coreboot[master]: drivers/i2c/nct7802y: Configure remote diodes and local sensor

Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39766 )

Change subject: drivers/i2c/nct7802y: Configure remote diodes and local sensor ......................................................................

Patch Set 32:

(3 comments)

https://review.coreboot.org/c/coreboot/+/39766/31/src/drivers/i2c/nct7802y/n... File src/drivers/i2c/nct7802y/nct7802y_fan.c:

https://review.coreboot.org/c/coreboot/+/39766/31/src/drivers/i2c/nct7802y/n... PS31, Line 81: value = 0

...

Right, sounds good.

Done

https://review.coreboot.org/c/coreboot/+/39766/31/src/drivers/i2c/nct7802y/n... PS31, Line 82: (i << 1)

...

i * 2?

Done

https://review.coreboot.org/c/coreboot/+/39766/31/src/drivers/i2c/nct7802y/n... PS31, Line 82: i << 1

...

Please just write `i * 2`. This is not very readable. Also wouldn't need parentheses. […]

Done

-- To view, visit https://review.coreboot.org/c/coreboot/+/39766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I28cc4e5cae76cf0bcdad26a50ee6cd43a201d31e Gerrit-Change-Number: 39766 Gerrit-PatchSet: 32 Gerrit-Owner: Maxim Polyakov max.senia.poliak@gmail.com Gerrit-Reviewer: Felix Singer felixsinger@posteo.net Gerrit-Reviewer: Andrey Petrov andrey.petrov@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Lance Zhao Gerrit-Reviewer: Mario Scheithauer mario.scheithauer@siemens.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Werner Zeh werner.zeh@siemens.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Mon, 28 Sep 2020 12:15:41 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber nico.h@gmx.de Comment-In-Reply-To: Angel Pons th3fanbus@gmail.com Gerrit-MessageType: comment

Maxim Polyakov (Code Review)

1 Oct 1 Oct

1:34 p.m.

New subject: Change in coreboot[master]: drivers/i2c/nct7802y: Configure remote diodes and local sensor

Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39766 )

Change subject: drivers/i2c/nct7802y: Configure remote diodes and local sensor ......................................................................

Patch Set 33:

Anything else? Is it ready for +2?

-- To view, visit https://review.coreboot.org/c/coreboot/+/39766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I28cc4e5cae76cf0bcdad26a50ee6cd43a201d31e Gerrit-Change-Number: 39766 Gerrit-PatchSet: 33 Gerrit-Owner: Maxim Polyakov max.senia.poliak@gmail.com Gerrit-Reviewer: Felix Singer felixsinger@posteo.net Gerrit-Reviewer: Andrey Petrov andrey.petrov@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Lance Zhao Gerrit-Reviewer: Mario Scheithauer mario.scheithauer@siemens.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Werner Zeh werner.zeh@siemens.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Thu, 01 Oct 2020 11:34:46 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Werner Zeh (Code Review)

1:52 p.m.

New subject: Change in coreboot[master]: drivers/i2c/nct7802y: Configure remote diodes and local sensor

Werner Zeh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39766 )

Change subject: drivers/i2c/nct7802y: Configure remote diodes and local sensor ......................................................................

Patch Set 33: Code-Review+2

-- To view, visit https://review.coreboot.org/c/coreboot/+/39766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I28cc4e5cae76cf0bcdad26a50ee6cd43a201d31e Gerrit-Change-Number: 39766 Gerrit-PatchSet: 33 Gerrit-Owner: Maxim Polyakov max.senia.poliak@gmail.com Gerrit-Reviewer: Felix Singer felixsinger@posteo.net Gerrit-Reviewer: Andrey Petrov andrey.petrov@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Lance Zhao Gerrit-Reviewer: Mario Scheithauer mario.scheithauer@siemens.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Werner Zeh werner.zeh@siemens.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Thu, 01 Oct 2020 11:52:22 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

Maxim Polyakov (Code Review)

3:02 p.m.

New subject: Change in coreboot[master]: drivers/i2c/nct7802y: Configure remote diodes and local sensor

Maxim Polyakov has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39766 )

Change subject: drivers/i2c/nct7802y: Configure remote diodes and local sensor ......................................................................

Patch Set 33:

...

Patch Set 33:

Anything else? Is it ready for +2?

Thanks!

-- To view, visit https://review.coreboot.org/c/coreboot/+/39766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I28cc4e5cae76cf0bcdad26a50ee6cd43a201d31e Gerrit-Change-Number: 39766 Gerrit-PatchSet: 33 Gerrit-Owner: Maxim Polyakov max.senia.poliak@gmail.com Gerrit-Reviewer: Felix Singer felixsinger@posteo.net Gerrit-Reviewer: Andrey Petrov andrey.petrov@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Lance Zhao Gerrit-Reviewer: Mario Scheithauer mario.scheithauer@siemens.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Werner Zeh werner.zeh@siemens.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Thu, 01 Oct 2020 13:02:31 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Nico Huber (Code Review)

11 Oct 11 Oct

1:24 p.m.

New subject: Change in coreboot[master]: drivers/i2c/nct7802y: Configure remote diodes and local sensor

Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39766 )

Change subject: drivers/i2c/nct7802y: Configure remote diodes and local sensor ......................................................................

Patch Set 33:

...

Anything else? Is it ready for +2?

Well, looks like my comment on the location (file) was ignored.

-- To view, visit https://review.coreboot.org/c/coreboot/+/39766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I28cc4e5cae76cf0bcdad26a50ee6cd43a201d31e Gerrit-Change-Number: 39766 Gerrit-PatchSet: 33 Gerrit-Owner: Maxim Polyakov max.senia.poliak@gmail.com Gerrit-Reviewer: Felix Singer felixsinger@posteo.net Gerrit-Reviewer: Andrey Petrov andrey.petrov@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Lance Zhao Gerrit-Reviewer: Mario Scheithauer mario.scheithauer@siemens.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Werner Zeh werner.zeh@siemens.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-Comment-Date: Sun, 11 Oct 2020 11:24:13 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

Nico Huber (Code Review)

1:24 p.m.

New subject: Change in coreboot[master]: drivers/i2c/nct7802y: Configure remote diodes and local sensor

Nico Huber has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39766 )

Change subject: drivers/i2c/nct7802y: Configure remote diodes and local sensor ......................................................................

drivers/i2c/nct7802y: Configure remote diodes and local sensor

TEST = After loading the nct7802 module on the Kontron mAL-10 [2] with Linux OS, we can see configuration of the HWM with one sensor in the thermistor mode:

[1] page 30, section 7.2.32, Nuvoton Hardware Monitoring IC NCT7802Y with PECI 3.0 interface, datasheet, revision 1.2, february 2012 [2] https://review.coreboot.org/c/coreboot/+/39133

Change-Id: I28cc4e5cae76cf0bcdad26a50ee6cd43a201d31e Signed-off-by: Maxim Polyakov max.senia.poliak@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/39766 Reviewed-by: Werner Zeh werner.zeh@siemens.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/drivers/i2c/nct7802y/chip.h M src/drivers/i2c/nct7802y/nct7802y.h M src/drivers/i2c/nct7802y/nct7802y_fan.c 3 files changed, 31 insertions(+), 7 deletions(-)

Approvals: build bot (Jenkins): Verified Werner Zeh: Looks good to me, approved

diff --git a/src/drivers/i2c/nct7802y/chip.h b/src/drivers/i2c/nct7802y/chip.h index 925dca1..03c464a 100644 --- a/src/drivers/i2c/nct7802y/chip.h +++ b/src/drivers/i2c/nct7802y/chip.h @@ -7,6 +7,15 @@

#define NCT7802Y_PECI_CNT 2 #define NCT7802Y_FAN_CNT 3 +#define NCT7802Y_RTD_CNT 3 + +/* Remote temperature diode sensors mode */ +enum nct7802y_rtd_mode { + RTD_CLOSED = 0, + RTD_CURRENT_MODE, + RTD_THERMISTOR_MODE, + RTD_VOLTAGE_MODE, +};

enum nct7802y_peci_mode { PECI_DISABLED = 0, @@ -53,6 +62,11 @@ TEMP_SOURCE_PROGRAMMABLE_1, };

+struct nct7802y_sensors_config { + bool local_enable; + enum nct7802y_rtd_mode rtd[NCT7802Y_RTD_CNT]; +}; + struct nct7802y_fan_smartconfig { enum nct7802y_fan_smartmode mode; enum nct7802y_fan_speed speed; @@ -76,6 +90,7 @@ struct drivers_i2c_nct7802y_config { struct nct7802y_peci_config peci[NCT7802Y_PECI_CNT]; struct nct7802y_fan_config fan[NCT7802Y_FAN_CNT]; + struct nct7802y_sensors_config sensors; enum nct7802y_fan_pecierror on_pecierror; u8 pecierror_minduty; }; diff --git a/src/drivers/i2c/nct7802y/nct7802y.h b/src/drivers/i2c/nct7802y/nct7802y.h index aa07e70..d9b5878 100644 --- a/src/drivers/i2c/nct7802y/nct7802y.h +++ b/src/drivers/i2c/nct7802y/nct7802y.h @@ -11,6 +11,9 @@ #define BANK_SELECT 0x00

/* Bank 0 */ +#define MODE_SELECTION 0x22 +#define MODE_SELECTION_LTD_EN (1 << 6) +#define MODE_SELECTION_RTDx(x, val) ((val) << (x) * 2)

#define PECI_ENABLE 0x23 #define PECI_ENABLE_AGENTx(x) (1 << (x)) diff --git a/src/drivers/i2c/nct7802y/nct7802y_fan.c b/src/drivers/i2c/nct7802y/nct7802y_fan.c index 7771b78..a608802 100644 --- a/src/drivers/i2c/nct7802y/nct7802y_fan.c +++ b/src/drivers/i2c/nct7802y/nct7802y_fan.c @@ -68,7 +68,7 @@ { const struct drivers_i2c_nct7802y_config *const config = dev->chip_info; unsigned int i; - u8 set; + u8 value;

if (nct7802y_select_bank(dev, 0) != CB_SUCCESS) return; @@ -78,21 +78,27 @@ init_fan(dev, &config->fan[i], i); }

+ value = 0; + for (i = 0; i < NCT7802Y_RTD_CNT; ++i) + value |= MODE_SELECTION_RTDx(i, config->sensors.rtd[i]); + if (config->sensors.local_enable) + value |= MODE_SELECTION_LTD_EN; + nct7802y_write(dev, MODE_SELECTION, value); + switch (config->on_pecierror) { case PECI_ERROR_KEEP: - set = CLOSE_LOOP_FAN_PECI_ERR_CURR; + value = CLOSE_LOOP_FAN_PECI_ERR_CURR; break; case PECI_ERROR_VALUE: - set = CLOSE_LOOP_FAN_PECI_ERR_VALUE; + value = CLOSE_LOOP_FAN_PECI_ERR_VALUE; break; case PECI_ERROR_FULLSPEED: - set = CLOSE_LOOP_FAN_PECI_ERR_MAX; + value = CLOSE_LOOP_FAN_PECI_ERR_MAX; break; default: - set = 0; + value = 0; break; } - nct7802y_update(dev, CLOSE_LOOP_FAN_RPM_CTRL, - CLOSE_LOOP_FAN_PECI_ERR_MASK, set); + nct7802y_update(dev, CLOSE_LOOP_FAN_RPM_CTRL, CLOSE_LOOP_FAN_PECI_ERR_MASK, value); nct7802y_write(dev, FAN_DUTY_ON_PECI_ERROR, config->pecierror_minduty); }

-- To view, visit https://review.coreboot.org/c/coreboot/+/39766 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I28cc4e5cae76cf0bcdad26a50ee6cd43a201d31e Gerrit-Change-Number: 39766 Gerrit-PatchSet: 34 Gerrit-Owner: Maxim Polyakov max.senia.poliak@gmail.com Gerrit-Reviewer: Felix Singer felixsinger@posteo.net Gerrit-Reviewer: Andrey Petrov andrey.petrov@gmail.com Gerrit-Reviewer: Angel Pons th3fanbus@gmail.com Gerrit-Reviewer: Lance Zhao Gerrit-Reviewer: Mario Scheithauer mario.scheithauer@siemens.com Gerrit-Reviewer: Nico Huber nico.h@gmx.de Gerrit-Reviewer: Werner Zeh werner.zeh@siemens.com Gerrit-Reviewer: build bot (Jenkins) no-reply@coreboot.org Gerrit-CC: Paul Menzel paulepanter@users.sourceforge.net Gerrit-MessageType: merged

1557

days inactive

1760

days old

coreboot-gerrit@coreboot.org

28 comments

7 participants

tags (0)

participants (7)

Andrey Petrov (Code Review)
Angel Pons (Code Review)
build bot (Jenkins) (Code Review)
Mario Scheithauer (Code Review)
Maxim Polyakov (Code Review)
Nico Huber (Code Review)
Werner Zeh (Code Review)