Re: [PATCH v8 2/2] hwmon: temperature: add support for EMC1812

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Changing the temperature range format to offset-64 without rewriting limit registers causes spurious THERM pin assertions.
- [Medium] Unconditional write to the EXT2 Beta Configuration register on chips lacking an EXT2 channel.
- [Medium] Driver erroneously rejects valid device trees that fully describe all physical channels.
--

--- Patch [2]: [PATCH v8 2/2] hwmon: temperature: add support for EMC1812 ---
commit 916836a5ccfc6cb9e93591ec739ea81756bca76b
Author: Marius Cristea <marius.cristea@microchip.com>
Date:   Tue Mar 10 14:50:46 2026 +0000

    hwmon: temperature: add support for EMC1812
    
    This is the hwmon driver for Microchip EMC1812/13/14/15/33
    Multichannel Low-Voltage Remote Diode Sensor Family.

> diff --git a/drivers/hwmon/emc1812.c b/drivers/hwmon/emc1812.c
> --- /dev/null
> +++ b/drivers/hwmon/emc1812.c
[ ... ]
> +static int emc1812_init(struct emc1812_data *priv)
> +{
> +	int ret;
> +	u8 val;
> +
> +	/*
> +	 * Set default values in registers. APDD, RECD12 and RECD34 are active
> +	 * on 0. Set ALERT pin to be in comparator mode.
> +	 * Set the device to be in Run (Active) state and converting on all
> +	 * channels.
> +	 * Don't change conversion rate. After reset, default is 4 conversions/seconds.
> +	 * The temperature measurement range is -64°C to +191.875°C.
> +	 */
> +	val = FIELD_PREP(EMC1812_CFG_MSKAL, 1) |
> +	      FIELD_PREP(EMC1812_CFG_RS, 0) |
> +	      FIELD_PREP(EMC1812_CFG_ATTHM, 1) |
> +	      FIELD_PREP(EMC1812_CFG_RECD12, !priv->recd12_en) |
> +	      FIELD_PREP(EMC1812_CFG_RECD34, !priv->recd34_en) |
> +	      FIELD_PREP(EMC1812_CFG_RANGE, 1) |
> +	      FIELD_PREP(EMC1812_CFG_DA_ENA, 0) |
> +	      FIELD_PREP(EMC1812_CFG_APDD, !priv->apdd_en);
> +
> +	ret = regmap_write(priv->regmap, EMC1812_CFG_ADDR, val);

[Severity: High]
When setting the EMC1812_CFG_RANGE bit to 1, the data format changes to a
-64 to 191 degree range. If the limit registers aren't rewritten to match this
new offset, will the existing defaults (such as 85 degrees) be misinterpreted
by the hardware as a much lower temperature (like 21 degrees), causing spurious
thermal limit alarms or hardware shutdowns on driver load?

> +	if (ret)
> +		return ret;
[ ... ]
> +	/* Enables the beta compensation factor auto-detection function for beta1 and beta2 */
> +	ret = regmap_write(priv->regmap, EMC1812_EXT1_BETA_CONFIG_ADDR,
> +			   EMC1812_BETA_LOCK_VAL);
> +	if (ret)
> +		return ret;
> +
> +	ret = regmap_write(priv->regmap, EMC1812_EXT2_BETA_CONFIG_ADDR,
> +			   EMC1812_BETA_LOCK_VAL);

[Severity: Medium]
Does this unconditional write to EMC1812_EXT2_BETA_CONFIG_ADDR cause
communication errors or unpredictable behavior on chips like the EMC1812
that only have one external channel?

Unlike the ideality factor logic below, there doesn't seem to be a check
for the number of supported channels here before writing to the second
external channel's configuration register.

> +	if (ret)
> +		return ret;
[ ... ]
> +static int emc1812_parse_fw_config(struct emc1812_data *data, struct device *dev)
> +{
> +	unsigned int reg_nr = 0;
> +	int num_channels, ret;
> +
> +	/* To be able to load the driver in case we don't have device tree */
> +	if (!dev_fwnode(dev)) {
> +		data->active_ch_mask = BIT(data->chip->phys_channels) - 1;
> +		return 0;
> +	}
> +
> +	data->apdd_en = device_property_read_bool(dev, "microchip,enable-anti-parallel");
> +	data->recd12_en = device_property_read_bool(dev, "microchip,parasitic-res-on-channel1-2");
> +	data->recd34_en = device_property_read_bool(dev, "microchip,parasitic-res-on-channel3-4");
> +
> +	num_channels = device_get_child_node_count(dev) + 1;
> +
> +	if (num_channels > data->chip->phys_channels)
> +		return dev_err_probe(dev, -EINVAL, "More channels than the chip supports\n");

[Severity: Medium]
If a device tree explicitly defines a node for the internal channel (reg=0),
will this reject the valid configuration?

By unconditionally adding 1 to device_get_child_node_count(), num_channels
will equal phys_channels + 1 for a fully described device tree, which seems
like it would erroneously trigger this error path.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260310-hw_mon-emc1812-v8-0-bc155727e0d2@microchip.com?part=2