Re: [PATCH v2 2/2] iio: temperature: add support for MCP998X

[Andy Shevchenko <andy.shevchenko@gmail.com>]

On Thu, May 29, 2025 at 12:37 PM <victor.duicu@microchip.com> wrote:
>
> This is the driver for Microchip MCP998X/33 and MCP998XD/33D
> Multichannel Automotive Temperature Monitor Family.

...

> +/*
> + * (Sampling_Frequency * 1000000) / (Window_Size * 2)
> + */
> +static int mcp9982_calc_all_3db_values(void)
> +{

> +       int i, j;

Why signed?

> +       u64 numerator;
> +       u32 denominator, remainder;
> +
> +       for (i = 0; i < ARRAY_SIZE(mcp9982_window_size); i++)
> +               for (j = 0; j <  ARRAY_SIZE(mcp9982_sampl_fr); j++) {

> +                       numerator = 1000000 * mcp9982_sampl_fr[j][0];

MICRO ? Ditto for the below case.

> +                       denominator = 2 * mcp9982_window_size[i] * mcp9982_sampl_fr[j][1];
> +                       numerator = div_u64_rem(numerator, denominator, &remainder);

The remainder seems unused here. So, why do you use the div_u64_rem()
and not simply do_div()?

> +                       mcp9982_3db_values_map_tbl[j][i][0] = div_u64_rem(numerator, 1000000,
> +                                                                         &remainder);
> +                       mcp9982_3db_values_map_tbl[j][i][1] = remainder;
> +               }
> +       return 0;
> +}

...

> +struct mcp9982_priv {
> +       u8 num_channels;
> +       bool extended_temp_range;
> +       bool beta_autodetect[2];
> +       /*
> +        * Synchronize access to private members, and ensure
> +        * atomicity of consecutive regmap operations.
> +        */
> +       struct mutex lock;

> +       struct regmap *regmap;

Wouldn't moving this to be the first member help with the code
generation (size)?

> +       struct iio_chan_spec *iio_chan;
> +       char *labels[MCP9982_MAX_NUM_CHANNELS];
> +       int ideality_value[4];
> +       int recd34_enable;
> +       int recd12_enable;
> +       int apdd_enable;
> +       int sampl_idx;
> +};

...

> +static int mcp9982_read_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> +                           int *val, int *val2, long mask)
> +{
> +       struct mcp9982_priv *priv = iio_priv(indio_dev);
> +       int ret, index, index2;

regmap API takes unsigned int, why are these signed? And in general
it's better not to mix the returning variable with something
semantically different.

> +       ret = regmap_write(priv->regmap, MCP9982_ONE_SHOT_ADDR, 1);
> +       if (ret)
> +               return ret;
> +
> +       guard(mutex)(&priv->lock);
> +
> +       switch (mask) {
> +       case IIO_CHAN_INFO_RAW:
> +               ret = regmap_read(priv->regmap, MCP9982_INT_VALUE_ADDR(chan->channel), val);
> +               if (ret)
> +                       return ret;
> +
> +               /* The extended temperature range is offset by 64 degrees C */
> +               if (priv->extended_temp_range)
> +                       *val -= 64;
> +
> +               ret = regmap_read(priv->regmap, MCP9982_FRAC_VALUE_ADDR(chan->channel), val2);
> +               if (ret)
> +                       return ret;
> +
> +               /* Only the 3 MSB in low byte registers are used */
> +               *val2 = mcp9982_fractional_values[*val2 >> 5];
> +               return IIO_VAL_INT_PLUS_MICRO;
> +       case IIO_CHAN_INFO_SAMP_FREQ:
> +               *val = mcp9982_conv_rate[priv->sampl_idx][0];
> +               *val2 = mcp9982_conv_rate[priv->sampl_idx][1];
> +               return IIO_VAL_INT_PLUS_MICRO;
> +       case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> +
> +               ret = regmap_read(priv->regmap, MCP9982_RUNNING_AVG_ADDR, &index2);
> +               if (ret)
> +                       return ret;
> +
> +               if (index2 >= 2)
> +                       index2 -= 1;

Sounds like a clamp(). (Note, what if for whatever reason you will get
index2 bigger than array size?

> +               *val = mcp9982_3db_values_map_tbl[priv->sampl_idx][index2][0];
> +               *val2 = mcp9982_3db_values_map_tbl[priv->sampl_idx][index2][1];
> +               return IIO_VAL_INT_PLUS_MICRO;
> +       case IIO_CHAN_INFO_HYSTERESIS:
> +               ret = regmap_read(priv->regmap, MCP9982_HYS_ADDR, &index);
> +               if (ret)
> +                       return ret;
> +
> +               *val = index;
> +               return IIO_VAL_INT;
> +       default:
> +               return -EINVAL;
> +       }
> +}

...

> +static int mcp9982_write_raw(struct iio_dev *indio_dev, struct iio_chan_spec const *chan,
> +                            int val, int val2, long mask)
> +{
> +       struct mcp9982_priv *priv = iio_priv(indio_dev);
> +       int i, ret;

Why is 'i' signed?

> +       guard(mutex)(&priv->lock);
> +       switch (mask) {
> +       case IIO_CHAN_INFO_SAMP_FREQ:
> +               for (i = 0; i < ARRAY_SIZE(mcp9982_conv_rate); i++)
> +                       if (val == mcp9982_conv_rate[i][0] && val2 == mcp9982_conv_rate[i][1])
> +                               break;

> +               if (i >= ARRAY_SIZE(mcp9982_conv_rate))

What is the meaning of '>' here?

> +                       return -EINVAL;

> +               ret = regmap_write(priv->regmap, MCP9982_CONV_ADDR, i);
> +               if (ret)
> +                       return ret;
> +
> +               priv->sampl_idx = i;
> +               return 0;
> +       case IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY:
> +               for (i = 0; i < ARRAY_SIZE(mcp9982_3db_values_map_tbl[priv->sampl_idx]); i++)
> +                       if (val == mcp9982_3db_values_map_tbl[priv->sampl_idx][i][0] &&
> +                           val2 == mcp9982_3db_values_map_tbl[priv->sampl_idx][i][1])
> +                               break;

> +               if (i >= ARRAY_SIZE(mcp9982_3db_values_map_tbl[priv->sampl_idx]))
> +                       return -EINVAL;

Ditto.

> +               /*
> +                * Filter register is coded with values:
> +                *-0 for OFF
> +                *-1 or 2 for level 1
> +                *-3 for level 2

This lists the negative values.... Are you sure the comment is aligned
with what the code is really doing?

> +                */
> +               if (i == 2)
> +                       i = 3;
> +               ret = regmap_write(priv->regmap, MCP9982_RUNNING_AVG_ADDR, i);
> +
> +               return ret;
> +       case IIO_CHAN_INFO_HYSTERESIS:
> +               if (val < 0 || val > 255)
> +                       return -EINVAL;
> +
> +               ret = regmap_write(priv->regmap, MCP9982_HYS_ADDR, val);
> +               return ret;
> +       default:
> +               return -EINVAL;
> +       }
> +}

...

> +static ssize_t mcp9982_extended_temp_range_store(struct device *dev,
> +                                                struct device_attribute *attr,
> +                                                const char *buf, size_t count)
> +{
> +       struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +       struct mcp9982_priv *priv = iio_priv(indio_dev);
> +       int ret, val;
> +
> +       ret = kstrtouint(buf, 10, &val);
> +       if (ret)
> +               return -EINVAL;

Why is the error code shadowed?

> +       switch (val) {
> +       case 0:
> +               priv->extended_temp_range = false;
> +               break;
> +       case 1:
> +               priv->extended_temp_range = true;
> +               break;
> +       default:
> +               return -EINVAL;
> +       }

Useless, use kstrtobool() instead.

> +       guard(mutex)(&priv->lock);
> +       ret = regmap_assign_bits(priv->regmap, MCP9982_CFG_ADDR, MCP9982_CFG_RANGE,
> +                                priv->extended_temp_range);
> +
> +       if (ret)
> +               return ret;
> +
> +       return count;
> +}

...

> +static ssize_t mcp9982_show_beta(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +       struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +       struct mcp9982_priv *priv = iio_priv(indio_dev);
> +       struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);
> +       int val, ret;

Why is val signed? Please, check your code and fix types all over the place.

> +       /* When APDD is enabled, betas are locked to autodetection */
> +       if (priv->apdd_enable)
> +               return sysfs_emit(buf, "Auto\n");
> +
> +       ret = regmap_read(priv->regmap, MCP9982_EXT_BETA_CFG_ADDR(this_attr->address), &val);
> +       if (ret)
> +               return ret;
> +
> +       if (val < 15)
> +               return sysfs_emit(buf, "%s\n", mcp9982_beta_values[val]);

> +       if (val == 15)
> +               return sysfs_emit(buf, "Diode_Mode\n");

> +       else

Redundant 'else'.

> +               return sysfs_emit(buf, "Auto\n");
> +}
> +
> +static ssize_t mcp9982_store_beta(struct device *dev, struct device_attribute *attr,
> +                                 const char *buf, size_t count)
> +{
> +       struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> +       struct mcp9982_priv *priv = iio_priv(indio_dev);
> +       struct iio_dev_attr *this_attr = to_iio_dev_attr(attr);

> +       int i;

Signed? Why?

> +       /* When APDD is enabled, betas are locked to autodetection */
> +       if (priv->apdd_enable)
> +               return -EINVAL;

The below looks like an attempt to reimplement sysfs_match_string().

> +       for (i = 0; i < ARRAY_SIZE(mcp9982_beta_values); i++)
> +               if (strncmp(buf, mcp9982_beta_values[i], 5) == 0)
> +                       break;
> +
> +       if (i < ARRAY_SIZE(mcp9982_beta_values)) {
> +               regmap_write(priv->regmap, MCP9982_EXT_BETA_CFG_ADDR(this_attr->address), i);
> +               return count;
> +       }
> +
> +       if (strncmp(buf, "Diode_Mode", 10) == 0) {
> +               regmap_write(priv->regmap, MCP9982_EXT_BETA_CFG_ADDR(this_attr->address), 15);
> +               return count;
> +       }
> +
> +       if (strncmp(buf, "Auto", 4) == 0) {
> +               regmap_write(priv->regmap, MCP9982_EXT_BETA_CFG_ADDR(this_attr->address), BIT(4));
> +               return count;
> +       }
> +
> +       return -EINVAL;
> +}
> +
> +static ssize_t mcp9982_beta_available_show(struct device *dev,
> +                                          struct device_attribute *attr, char *buf)
> +{
> +       int i;
> +
> +       for (i = 0; i < 15; i++) {

> +               strcat(buf, mcp9982_beta_values[i]);
> +               strcat(buf, " ");

Huh?!

> +       }
> +       strcat(buf, "Diode_Mode Auto\n");
> +       return sysfs_emit(buf, buf);

What does this mean, please?

> +}
> +
> +static IIO_DEVICE_ATTR(enable_extended_temp_range, 0644, mcp9982_extended_temp_range_show,
> +                      mcp9982_extended_temp_range_store, 0);
> +static IIO_DEVICE_ATTR(in_beta1, 0644, mcp9982_show_beta, mcp9982_store_beta, 0);
> +static IIO_DEVICE_ATTR(in_beta2, 0644, mcp9982_show_beta, mcp9982_store_beta, 1);
> +static IIO_DEVICE_ATTR(in_beta_available, 0444, mcp9982_beta_available_show, NULL, 0);

First of all, we have IIO_DEVICE_ATTR_RO/RW, second, move each of them
to be closer to the related callback(s).

...

> +static struct attribute *mcp9982_attributes[] = {
> +       &iio_dev_attr_enable_extended_temp_range.dev_attr.attr,
> +       &iio_dev_attr_in_beta1.dev_attr.attr,
> +       &iio_dev_attr_in_beta2.dev_attr.attr,
> +       &iio_dev_attr_in_beta_available.dev_attr.attr,
> +       NULL,

No comma for the terminator.

> +};

I stop there as it reminds me that I had commented on something
similar in the past and nothing here was following those comments. Am
I correct?

--
With Best Regards,
Andy Shevchenko