| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <402ba937-226d-44be-8411-428a8842a35c@roeck-us.net>
Date: Thu, 5 Feb 2026 17:09:45 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Marius Cristea <marius.cristea@...rochip.com>
Cc: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>,
Conor Dooley <conor+dt@...nel.org>,
Jonathan Corbet <corbet@....net>, linux-hwmon@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-doc@...r.kernel.org
Subject: Re: [PATCH v5 2/2] hwmon: temperature: add support for EMC1812
On Thu, Feb 05, 2026 at 09:09:04AM +0200, Marius Cristea wrote:
> This is the hwmon driver for Microchip EMC1812/13/14/15/33
> Multichannel Low-Voltage Remote Diode Sensor Family.
>
> EMC1812 has one external remote temperature monitoring channel.
> EMC1813 has two external remote temperature monitoring channels.
> EMC1814 has three external remote temperature monitoring channels and
> channels 2 and 3 supports anti parallel diode.
> EMC1815 has four external remote temperature monitoring channels and
> channels 1/2 and 3/4 supports anti parallel diode.
> EMC1833 has two external remote temperature monitoring channels and
> channels 1 and 2 supports anti parallel diode.
>
> Signed-off-by: Marius Cristea <marius.cristea@...rochip.com>
> ---
Additional AI generated review feedback below. Please address.
Thanks,
Guenter
---
# Task
Date: 2026-02-05 16:00:08
Model: gemini-3-flash-preview
Prompts SHA: 52e5df6 ("various changes")
Commits to review:
- 0f6303cffaab ("dt-bindings: hwmon: temperature: add support for EMC1812")
- c5a5b0f370f0 ("hwmon: temperature: add support for EMC1812")
# Commit 0f6303cffaab ("dt-bindings: hwmon: temperature: add support for EMC1812")
commit 0f6303cffaabd0f20f74335ad88e6e3ec5876d93
Author: Marius Cristea <marius.cristea@...rochip.com>
dt-bindings: hwmon: temperature: add support for EMC1812
This commit adds the DeviceTree schema for the Microchip EMC1812 family
of temperature sensors and updates the MAINTAINERS file.
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/hwmon/microchip,emc1812.yaml
[ ... ]
> + interrupts:
> + items:
> + - description: alert-therm2 asserts when the ALERT limit is exceeded.
> + - description: therm-addr asserts when the THERM limit is exceeded.
> + minItems: 1
> +
> + interrupt-names:
> + items:
> + - enum: [alert-therm2, therm-addr]
Is the interrupt-names schema intended to be this flexible?
The current items definition only constrains the first element of the list.
If a second interrupt is provided, it remains unconstrained.
Additionally, if only one interrupt is provided, the interrupts property
enforces it to be alert-therm2 (the first positional item), but the
interrupt-names enum allows it to be named therm-addr.
Should it be defined as:
interrupt-names:
items:
- const: alert-therm2
- const: therm-addr
minItems: 1
[ ... ]
> + microchip,parasitic-res-on-channel1-2:
> + description:
> + Indicates that the chip and the diodes/transistors are sufficiently
> + far apart that a parasitic resistance is added to the wires, which can
> + affect the measurements. Due to the anti-parallel diode connections,
> + channels 1 and 2 are affected together.
> + type: boolean
Is this description accurate for all supported models?
For the EMC1812, there is only one external channel, so it seems
unlikely that "channels 1 and 2 are affected together".
Also, the EMC1813 does not appear to support anti-parallel diodes
according to the commit message and driver documentation, yet this
description attributes the property to anti-parallel diode connections.
[ ... ]
> +patternProperties:
> + "^channel@[1-4]$":
[ ... ]
> + properties:
> + reg:
> + items:
> + minimum: 1
> + maximum: 4
Does the reg property for channels need a maxItems: 1 constraint?
Without it, the schema allows reg to be an array of multiple values,
whereas a channel node reg should typically be a single value.
[ ... ]
> +allOf:
> + - if:
> + properties:
> + compatible:
> + contains:
> + enum:
> + - microchip,emc1812
> + - microchip,emc1813
> + - microchip,emc1833
> + then:
> + properties:
> + microchip,parasitic-res-on-channel3-4: false
The EMC1814 supports anti-parallel diodes on channels 2 and 3, but only
has three external channels in total.
Should microchip,parasitic-res-on-channel3-4 also be disabled for the
EMC1814, since it doesn't have a fourth channel?
If the EMC1814 uses this property for its anti-parallel pair (2 and 3),
the property name appears to be a mismatch.
[ ... ]
The driver added in commit c5a5b0f370f0 references a property named
"microchip,enable-anti-parallel", but it is missing from this schema.
Should "microchip,enable-anti-parallel" be added to the bindings?
[ ... ]
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 0efa8cc6775b..c807025d911e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
# Commit c5a5b0f370f0 ("hwmon: temperature: add support for EMC1812")
commit c5a5b0f370f04e9e4ae78ae82077cfb684461d8e
Author: Marius Cristea <marius.cristea@...rochip.com>
hwmon: temperature: add support for EMC1812
This commit adds a new hwmon driver for the Microchip EMC1812 family of
temperature sensors, including support for EMC1812, EMC1813, EMC1814,
EMC1815, and EMC1833 chips.
> diff --git a/Documentation/hwmon/emc1812.rst b/Documentation/hwmon/emc1812.rst
> new file mode 100644
> index 000000000000..799111a89541
> --- /dev/null
> +++ b/Documentation/hwmon/emc1812.rst
> @@ -0,0 +1,68 @@
> +.. SPDX-License-Identifier: GPL-2.0-or-later
> +
> +Kernel driver emc1802
> +=====================
Should this be emc1812?
> +Author:
> + Marius Cristea <marius.cristea@...rochip.com
The closing angle bracket seems to be missing from the email address.
[ ... ]
> diff --git a/drivers/hwmon/emc1812.c b/drivers/hwmon/emc1812.c
> new file mode 100644
> index 000000000000..30bbc377d592
> --- /dev/null
> +++ b/drivers/hwmon/emc1812.c
[ ... ]
> +static int emc1812_read(struct device *dev, enum hwmon_sensor_types type, u32 attr,
> + int channel, long *val)
> +{
> + struct emc1812_data *data = dev_get_drvdata(dev);
> + unsigned int convrate;
> + int ret;
> +
> + switch (type) {
> + case hwmon_temp:
> + return emc1812_read_reg(dev, data, attr, channel, val);
> + case hwmon_chip:
> + switch (attr) {
> + case hwmon_chip_update_interval:
> + ret = regmap_read(data->regmap, EMC1812_CONV_ADDR, &convrate);
> + if (ret < 0)
> + return ret;
> +
> + if (convrate > 10)
> + convrate = 4;
> +
> + *val = 16000 >> convrate;
If convrate is 10, *val becomes 15. However, emc1812_conv_time[10] is 16.
This results in a slight discrepancy where writing 16ms and reading it back
returns 15ms.
> + return 0;
> + default:
> + return -EOPNOTSUPP;
> + }
> + default:
> + return -EOPNOTSUPP;
> + }
> +}
[ ... ]
> +static int emc1812_set_temp(struct device *dev, struct emc1812_data *data, int channel,
> + enum emc1812_limit_type map, long val)
> +{
> + long valh, vall;
> + u8 regh, regl;
> + int ret;
> +
> + regh = emc1812_limit_regs[channel][map];
> + regl = emc1812_limit_regs_low[channel][map];
> +
> + if (channel) {
> + val = DIV_ROUND_CLOSEST(val, 125);
> + valh = (val >> 3) & 0xff;
> + vall = (val & 0x07) << 5;
> + } else {
> + /* internal channel limit is stored on 8bits */
> + valh = DIV_ROUND_CLOSEST(val, 1000);
> + }
> +
> + ret = regmap_write(data->regmap, regh, valh);
Can this write overflow the 8-bit register for the internal channel?
In emc1812_write(), val is clamped to a maximum of 191875 and then offset
by 64000, resulting in 255875.
emc1812_write()
val = clamp_val(val, -64000, 191875);
val = val + 64000; // val is 255875
emc1812_set_temp()
valh = DIV_ROUND_CLOSEST(255875, 1000); // valh is 256
Since the register is 8 bits wide, writing 256 (0x100) will be truncated
to 0x00, setting the limit to -64.0 C instead of the requested 191.875 C.
[ ... ]
> +static int emc1812_init(struct emc1812_data *priv)
> +{
> + int ret;
> + u8 val;
[ ... ]
> + /* Set ideality factor for all external channels */
> + ret = regmap_write(priv->regmap, EMC1812_EXT1_IDEALITY_FACTOR_ADDR,
> + EMC1812_DEFAULT_IDEALITY_FACTOR);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(priv->regmap, EMC1812_EXT2_IDEALITY_FACTOR_ADDR,
> + EMC1812_DEFAULT_IDEALITY_FACTOR);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(priv->regmap, EMC1812_EXT3_IDEALITY_FACTOR_ADDR,
> + EMC1812_DEFAULT_IDEALITY_FACTOR);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(priv->regmap, EMC1812_EXT4_IDEALITY_FACTOR_ADDR,
> + EMC1812_DEFAULT_IDEALITY_FACTOR);
> + if (ret)
> + return ret;
Does this code need to write to EXT2/3/4 ideality factor registers if the
chip (like EMC1812) does not support those channels? While likely ignored
by the hardware, it seems redundant.
# Summary
| Commit | Regressions |
| :------------------------------------------------------------------------ | :---------- |
| 0f6303cffaab ("dt-bindings: hwmon: temperature: add support for EMC1812") | 6 |
| c5a5b0f370f0 ("hwmon: temperature: add support for EMC1812") | 5 |
Powered by blists - more mailing lists