| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHp75VeC8sWRpjKXc1Z1dvpSmCbz15ZB05daiOjdJaGiKGQ6wQ@mail.gmail.com>
Date: Thu, 29 May 2025 21:36:35 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: victor.duicu@...rochip.com
Cc: jic23@...nel.org, dlechner@...libre.com, nuno.sa@...log.com,
andy@...nel.org, robh@...nel.org, krzk+dt@...nel.org, conor+dt@...nel.org,
marius.cristea@...rochip.com, linux-iio@...r.kernel.org,
linux-kernel@...r.kernel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v2 2/2] iio: temperature: add support for MCP998X
On Thu, May 29, 2025 at 12:37 PM <victor.duicu@...rochip.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
Powered by blists - more mailing lists