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

[Guenter Roeck]

On 3/27/26 09:30, Marius.Cristea@xxxxxxxxxxxxx wrote:
> Hi Guenther,
>
>     Thanks for the review, please see my comments below:
>
> ...
>
> > > +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);
> > > +
> >
> > I assume it is on purpose that the defaults for EMC1812_CFG_RECD12
> > and
> > EMC1812_CFG_RECD34 deviate from the chip default (chip: enabled;
> > driver:
> > disabled).
>
> Yes, EMC1812_CFG_ATTHM was set in order for the alerts to be clear
> automaticaly when the limits goes back to normal.
>
> The EMC1812_CFG_RANGE is set to extended range in order to be able to
> measure from the -64 to 191,875 degree Celsius.
>
> The EMC1812_CFG_MSKAL could be left at the "reset", so I will change it
> to 0.
>
> The EMC1812_CFG_RECD12 and EMC1812_CFG_RECD34 will be set based on the
> device tree setting and is related to the hardware and if the system
> designer wants to enable or disable the resistance error correction.
>
>
> > > +     ret = regmap_write(priv->regmap, EMC1812_CFG_ADDR, val);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     ret = regmap_write(priv->regmap, EMC1812_THRM_HYS_ADDR,
> > > 0x0A);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     ret = regmap_write(priv->regmap, EMC1812_CONSEC_ALERT_ADDR,
> > > 0x70);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     ret = regmap_write(priv->regmap, EMC1812_FILTER_SEL_ADDR, 0);
> > > +     if (ret)
> > > +             return ret;
> > > +
> > > +     ret = regmap_write(priv->regmap, EMC1812_HOTTEST_CFG_ADDR,
> > > 0);
> > > +     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,
> >
> > AI review thinks that this register only exists on EMC1812. I don't
> > find that detail in the datasheet, but it is odd that there are two
> > registers
> > with supposedly the same functionality.
>
> All devices "have" the EMC1812_EXT2_BETA_CONFIG register (I mean if you
> are writing something to it, there will be no NAK on the i2c bus, but
> the value read back will be "0" for the devices that has the register
> not writable).
> EMC1812 having only one external channel, will not have the
> EMC1812_EXT2_BETA_CONFIG writable.
Ah, so the second register is for the second external channel. Seems
obvious, thinking about it ;-). Still, would it make sense to only write
the register if the second external channel actually exists ?

Thanks,
Guenter