Message-ID: <ad09b199f64336b7c5c07b2253b2c3f4bfef95d3.camel@microchip.com>
Date: Wed, 11 Feb 2026 10:51:04 +0000
From: <Marius.Cristea@...rochip.com>
To: <linux@...ck-us.net>
CC: <corbet@....net>, <linux-hwmon@...r.kernel.org>,
	<devicetree@...r.kernel.org>, <robh@...nel.org>,
	<linux-kernel@...r.kernel.org>, <krzk+dt@...nel.org>,
	<linux-doc@...r.kernel.org>, <conor+dt@...nel.org>
Subject: Re: [PATCH v5 2/2] hwmon: temperature: add support for EMC1812

Hi Guenter,

  Thanks you for the feedback. Please see my comments bellow.

Thanks,
Marius

On Thu, 2026-02-05 at 17:09 -0800, Guenter Roeck wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> 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")
> 
> 
> 
...
> > --- /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?
> 

Yes, my intention was to allow the user to use only the first, only the
second or both interrupts.
I will fix it.


> 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".

Here there is only one bit that will enable/disable the parasitic
resistance for one or a pair of 2 channels like "channel 1" or "channel
1 and 2".

In case of EMC1812 with only one external channel, this is the bit that
will control the resistance error correction (REC).

If the part supports two external channels I'm not able to control this
setting for each channel.

The naming came from the datasheet where "RECD1/2 bit disables the
resistance error correction for the DP1/DN1 and DP2/DN2 pins" so for
both channels at once independent of the anti-parallel configuration.


> 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.

Yes, the EMC1813 doesn't support anti-parallel diodes but it supports
the REC on both channels at once and can't be controlled individually.

...


> [ ... ]
> > +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?

"microchip,parasitic-res-on-channel3-4" should not be false for
EMC1814, because the part has 3 external channels.
REC for the channel 1 and 2 will be controlled by the
"microchip,parasitic-res-on-channel1-2" and for the third channel it
will be controlled by the "microchip,parasitic-res-on-channel3-4"


> 
> If the EMC1814 uses this property for its anti-parallel pair (2 and
> 3),
> the property name appears to be a mismatch.

anti-parallel is a separate property (there is only one bit per chip
that will control this functionality no matter how many external
channels are available)

Powered by blists - more mailing lists