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

[Marius.Cristea]

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.


Regards,
Marius