On Tue, Sep 19, 2023 at 12:34:52PM +0300, Daniel Matyas wrote:
> Used fwnode to retrieve data from the devicetree in the init_client
> function.
>
> If the uint32 properties are not present, the default values are used
> for max31827 chip.
>
> Signed-off-by: Daniel Matyas <daniel.matyas@xxxxxxxxxx>
> ---
[ ... ]
>
> +#define MAX31827_ALRM_POL_LOW 0x0
> +#define MAX31827_FLT_Q_1 0x0
> +
[ ... ]
> - return regmap_update_bits(st->regmap, MAX31827_CONFIGURATION_REG,
> - MAX31827_CONFIGURATION_1SHOT_MASK |
> - MAX31827_CONFIGURATION_CNV_RATE_MASK,
> - MAX31827_DEVICE_ENABLE(1));
> + res |= FIELD_PREP(MAX31827_CONFIGURATION_ALRM_POL_MASK, !!data);
> + } else {
> + res |= FIELD_PREP(MAX31827_CONFIGURATION_ALRM_POL_MASK,
> + MAX31827_ALRM_POL_LOW);
Since MAX31827_ALRM_POL_LOW is 0, this code doesn't really do
anything and just pollutes the code. Maybe that is acceptable
or even common in other subsystems, but I just find it confusing
and won't accept it. If a field is zero, leave it at that and
don't touch it.
> + }
> +
> + if (fwnode_property_present(fwnode, "adi,fault-q")) {
> + ret = fwnode_property_read_u32(fwnode, "adi,fault-q", &data);
> + if (ret)
> + return ret;
> +
> + /*
> + * Convert the desired fault queue into register bits.
> + */
> + lsb_idx = max31827__bf_shf(data);
> + if (lsb_idx > 3 || data != BIT(lsb_idx)) {
> + dev_err(&st->client->dev, "Invalid data in fault queue\n");
> + return -EOPNOTSUPP;
> + }
> +
> + res |= FIELD_PREP(MAX31827_CONFIGURATION_FLT_Q_MASK, lsb_idx);
> + } else {
> + res |= FIELD_PREP(MAX31827_CONFIGURATION_FLT_Q_MASK,
> + MAX31827_FLT_Q_1);
Same here and everywhere else where I missed it.
Guenter
