Re: [PATCH v9 0/2] Add support for Microchip EMC1812

[<Marius.Cristea@microchip.com>]

Hi Guenter,

  Unfortunately I didn't get the email in order to reply to it, so I
will add my comments here:

https://sashiko.dev/#/patchset/20260403-hw_mon-emc1812-v9-0-1a798f31cf2e%40microchip.com

On Fri, 2026-04-10 at 08:51 -0700, Guenter Roeck wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> On 4/3/26 05:39, Marius Cristea wrote:
> > This is the hwmon driver for EMC1812/13/14/15/33 multichannel Low-
> > Voltage
> > Remote Diode Sensor Family. The chips in the family have one
> > internal
> > and different numbers of external channels, ranging from 1
> > (EMC1812) to
> > 4 channels (EMC1815).
> > Reading diodes in anti-parallel connection is supported by EMC1814,
> > EMC1815
> > and EMC1833.
> > 
> > Signed-off-by: Marius Cristea <marius.cristea@microchip.com>
> 
> Sashiko still reports numberous issues which I consider valid:
> 
> https://sashiko.dev/#/patchset/20260403-hw_mon-emc1812-v9-0-1a798f31cf2e%40microchip.com
> 
> Please fix.
> 
> Thanks,
> Guenter
> 


> commit 80d00e3afb337f762ec753935e9c22a0db160357
> Author: Marius Cristea <marius.cristea@microchip.com>
> dt-bindings: hwmon: temperature: add support for EMC1812
>     
[ ... ]

> > +allOf:
> > +  # EMC1812: 1 Channel, No APD, REC on channel 1
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          const: microchip,emc1812
> > +    then:
> > +      properties:
> > +        microchip,enable-anti-parallel: false
> > +        microchip,parasitic-res-on-channel3-4: false
> Since EMC1812 only has 1 channel, should microchip,parasitic-res-on-
channel1-2
> also be forbidden here to prevent devicetrees from passing validation
if they
> specify parasitic resistance for the non-existent second channel?

Parasitic resistance correction (RECD12 bit) will work for both hannels
1 and 2, if both are available, or in the case the part has only the
channel 1 available it will be applied to it.

> [ ... ]
> > +  # EMC1814: 3 Channels, Supports APD,
> > +  # REC on both channel 1 & 2 and channel 3
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          const: microchip,emc1814
> > +    then:
> > +      properties:
> > +        channel@4: false
> Should microchip,parasitic-res-on-channel3-4 be set to false here as
well,
> since EMC1814 lacks a fourth channel?

Parasitic resistance correction (RECD34 bit) will work for both
channels 3 and 4, if both are available, or in the case the part has
only the channel 3 available it will be applied to it.

> Also, the other conditional blocks use patternProperties (e.g.,
"^channel@[2-4]$")
> to exclude nodes. To keep the exclusion syntax consistent, should
this block
> use patternProperties: "^channel@[4]$": false instead of properties?

My understanding was that we should not use "patternProperties" for
only one item, even for consistent look inside the file.


> commit 96ab5e76fb55f4a4e476765ae4ec35e5ddb7666f
> Author: Marius Cristea <marius.cristea@microchip.com>
> hwmon: temperature: add support for EMC1812
> This commit introduces the hardware monitoring driver for the
Microchip
> EMC1812/13/14/15/33 family of Multichannel Low-Voltage Remote Diode
> Sensors. It implements temperature reading, limit configuration, and
> hardware alarm handling.
> diff --git a/drivers/hwmon/emc1812.c b/drivers/hwmon/emc1812.c
> index 0000000000000..1ba8feac93b5f
> --- /dev/null
> +++ b/drivers/hwmon/emc1812.c
> [ ... ]
> > +static int emc1812_read_reg(struct device *dev, struct
emc1812_data *data, u32 attr,
> > +			    int channel, long *val)
> > +{
> [ ... ]
> > +	case hwmon_temp_min_alarm:
> > +		*val = regmap_test_bits(data->regmap,
EMC1812_LOW_LIMIT_STATUS_ADDR,
> > +					BIT(channel));
> > +		if (*val < 0)
> > +			return *val;
> > +
> > +		return 0;
> > +	case hwmon_temp_max_alarm:
> > +		*val = regmap_test_bits(data->regmap,
EMC1812_HIGH_LIMIT_STATUS_ADDR,
> > +					BIT(channel));
> Since status registers like EMC1812_LOW_LIMIT_STATUS_ADDR are read-
to-clear
> in hardware and marked as volatile in the regmap configuration,
reading the
> alarm for a single channel will perform a bus read that clears the
latched
> bits for all other channels simultaneously.
> If a userspace tool like sensors polls the channels sequentially,
will it
> permanently miss thermal events or faults on the other channels?
Should the
> driver implement software caching to preserve these clear-on-read
statuses?

Actually because the ALERT pin is set in "Comparator mode" the limit
status registers will be cleared only after one measurement is higher
then the low limit or below the limit minus the Therm hysteresis.
In "Comparator mode" the limit status resister will not be cleared by
reading it.

> [ ... ]

> > +	ret = regmap_write(priv->regmap, EMC1812_THRM_HYS_ADDR, 0x0A);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = regmap_write(priv->regmap, EMC1812_CONSEC_ALERT_ADDR,
0x70);
> The datasheet indicates that Bit 7 of the Consecutive ALERT register
(0x22)
> controls the SMBus timeout and defaults to 1 (enabled). Does writing
0x70
> here unintentionally clear Bit 7 and disable the hardware SMBus
timeout
> mechanism? 
> Should this use regmap_update_bits() instead to configure the
consecutive
> alert fields without altering the timeout configuration?

Into the datasheet page 56 the TMOUT is default to 0 (The I2C time-out
and Idle functionality are disabled).
Also on page 27 "The time-out function defaults to disabled. It can be
enabled by setting the TIMEOUT bit in the Consecutive Alert register."


Thanks and Best Regards,
Marius




> > ---
> > Changes in v9:
> > - improve the wording in the Documentation/hwmon/emc1812.rst file
> > - add const to variables in the driver
> > - initialize the EXT2_BETA_CONFIG only for the pats that support it
> > - update the writeble regmap table to exclude read-only registers
> > - Link to v8:
> > https://lore.kernel.org/r/20260310-hw_mon-emc1812-v8-0-bc155727e0d2@microchip.com
> >