On Thu, May 29, 2025 at 12:37 PM [off-list ref] 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