| 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: <CAHp75VdRisP+trez2Ysgrhan_zXMWsmawB3XeW+_ePsbNC4RzQ@mail.gmail.com>
Date: Sat, 14 Jun 2025 00:50:08 +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 v3 2/2] iio: temperature: add support for MCP998X
On Fri, Jun 13, 2025 at 4:02 PM <victor.duicu@...rochip.com> wrote:
>
> From: Victor Duicu <victor.duicu@...rochip.com>
>
> This is the driver for Microchip MCP998X/33 and MCP998XD/33D
> Multichannel Automotive Temperature Monitor Family.
...
> +MICROCHIP MCP9982 TEMPERATURE DRIVER
> +M: Victor Duicu <victor.duicu@...rochip.com>
> +L: linux-iio@...r.kernel.org
> +S: Supported
> +F: Documentation/devicetree/bindings/iio/temperature/microchip,mcp9982.yaml
> +F: drivers/iio/temperature/mcp9982.c
So, with the first patch only the dangling file will be present
without record in MAINTAINERS. Please, make sure that your DT schema
file is in MAINTAINERS.
> +config MCP9982
> + tristate "Microchip Technology MCP9982 driver"
> + depends on I2C
...
> +#include <asm/div64.h>
This needs to be linux/math64.h instead. The rule of thumb: prefer
linux/foo over asm/foo (with some exceptions, that are not the case
here).
...
> +#define MCP9982_INT_VALUE_ADDR(index) (2 * (index))
Maybe also ' + 0'? But I'm fine with this as well.
> +#define MCP9982_FRAC_VALUE_ADDR(index) (2 * (index) + 1)
...
> +#define MCP9982_EXT_IDEAL_ADDR(index) ((index) + 54)
What does 54 mean? What is the magic behind?
...
> +#define MCP9982_CHAN(index, si, __address) ({ \
> + struct iio_chan_spec __chan = { \
Why not compound literal?
> + .type = IIO_TEMP, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .info_mask_shared_by_all_available = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
> + BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) | \
> + BIT(IIO_CHAN_INFO_OFFSET), \
> + .info_mask_shared_by_all = BIT(IIO_CHAN_INFO_SAMP_FREQ) | \
> + BIT(IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY) | \
> + BIT(IIO_CHAN_INFO_HYSTERESIS) | \
> + BIT(IIO_CHAN_INFO_OFFSET), \
> + .channel = index, \
> + .address = __address, \
> + .scan_index = si, \
> + .scan_type = { \
> + .sign = 'u', \
> + .realbits = 8, \
> + .storagebits = 8, \
> + }, \
> + .indexed = 1, \
> + }; \
> + __chan; \
> +})
...
> +static const unsigned int mcp9982_window_size[3] = {1, 4, 8};
Add surrounding spaces inside {}.
...
> +/*
> + * (Sampling_Frequency * 1000000) / (Window_Size * 2)
This comment needs more elaboration, i.e. units in use for frequency
and perhaps window size.
> + */
> +static unsigned int mcp9982_calc_all_3db_values(void)
> +{
> + u32 denominator, remainder;
> + unsigned int i, j;
> + u64 numerator;
> +
> + for (i = 0; i < ARRAY_SIZE(mcp9982_window_size); i++)
> + for (j = 0; j < ARRAY_SIZE(mcp9982_sampl_fr); j++) {
Have you considered making mcp9982_sampl_fr to be struct u64_fract?
Also using here on stack something like
struct u64_fract tmp;
> + numerator = MICRO * mcp9982_sampl_fr[j][0];
> + denominator = 2 * mcp9982_window_size[i] * mcp9982_sampl_fr[j][1];
> + remainder = do_div(numerator, denominator);
> + remainder = do_div(numerator, MICRO);
> + mcp9982_3db_values_map_tbl[j][i][0] = numerator;
> + mcp9982_3db_values_map_tbl[j][i][1] = remainder;
The proposed changes will clarify the meaning of [0] and [1] in such a table.
> + }
> + return 0;
> +}
...
> +struct mcp9982_priv {
> + struct regmap *regmap;
> + u8 num_channels;
> + bool extended_temp_range;
> + bool recd34_enable;
> + bool recd12_enable;
> + unsigned int beta_values[2];
> + /*
> + * Synchronize access to private members, and ensure
> + * atomicity of consecutive regmap operations.
> + */
> + struct mutex lock;
> + struct iio_chan_spec *iio_chan;
> + const char *labels[MCP9982_MAX_NUM_CHANNELS];
> + unsigned int ideality_value[4];
> + unsigned int sampl_idx;
> + const char *dev_name;
> + bool apdd_enable;
Wouldn't it be slightly better to group all booleans (and move u8 here)?
> +};
...
> + if (strchr(priv->dev_name, 'd')) {
> + *vals = mcp9982_conv_rate[4];
> + *length = (ARRAY_SIZE(mcp9982_conv_rate) - 4) * 2;
> + } else {
> + *vals = mcp9982_conv_rate[0];
> + *length = ARRAY_SIZE(mcp9982_conv_rate) * 2;
> + }
So, the length can be multiplied only once here...
...
> + 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:
> +
Why this blank line?
> + ret = regmap_read(priv->regmap, MCP9982_RUNNING_AVG_ADDR, &tmp_reg);
> + if (ret)
> + return ret;
> + /*
> + * In Filter Selection Register values 1 and 2
> + * are mapped to the same setting.
> + */
> + switch (tmp_reg) {
> + case 0:
> + idx = 0;
> + break;
> + case 1:
> + case 2:
> + idx = 1;
> + break;
Instead of comment this can be regrouped like
case 0:
case 1:
idx = tmp_reg;
break;
case 2:
idx = 1;
break;
default:
...
> + default:
> + idx = 2;
> + break;
> + }
> +
> + *val = mcp9982_3db_values_map_tbl[priv->sampl_idx][idx][0];
> + *val2 = mcp9982_3db_values_map_tbl[priv->sampl_idx][idx][1];
> + return IIO_VAL_INT_PLUS_MICRO;
...
> +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 ret;
> + unsigned int i;
> + unsigned int start = 0;
> +
> + guard(mutex)(&priv->lock);
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + /*
> + * For MCP998XD and MCP9933D sampling frequency can't
> + * be set lower than 1.
> + */
> + if (strchr(priv->dev_name, 'd'))
Why not simply have this in an additional field of chip_info structure?
> + start = 4;
> + for (i = start; 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))
> + 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;
> +
> + /*
> + * In mcp9982_3db_values_map_tbl the second index maps:
> + * 0 for filter off
> + * 1 for filter at level 1
> + * 2 for filter at level 2
> + */
> + if (i == 2)
> + i = 3;
> + ret = regmap_write(priv->regmap, MCP9982_RUNNING_AVG_ADDR, i);
> +
> + return ret;
Why not
return regmap_write(...);
?
> + case IIO_CHAN_INFO_HYSTERESIS:
> + if (val < 0 || val > 255)
> + return -EINVAL;
> +
> + ret = regmap_write(priv->regmap, MCP9982_HYS_ADDR, val);
> + return ret;
Ditto.
> + case IIO_CHAN_INFO_OFFSET:
> + if (val != 0 && val != -64)
> + return -EINVAL;
> + priv->extended_temp_range = !(val == 0);
> + ret = regmap_assign_bits(priv->regmap, MCP9982_CFG_ADDR, MCP9982_CFG_RANGE,
> + priv->extended_temp_range);
> + return ret;
Ditto.
> + default:
> + return -EINVAL;
> + }
> +}
...
> +static int mcp9982_init(struct mcp9982_priv *priv)
> +{
> + int ret;
> + unsigned int i;
> + u8 val;
> +
> + /*
> + * For chips with "D" in the name
> + * set the below parameters to default to
> + * ensure that hardware shutdown feature
> + * can't be overridden.
> + */
> + if (strchr(priv->dev_name, 'd')) {
> + priv->recd12_enable = true;
> + priv->recd34_enable = true;
> + for (i = 0; i < 2; i++)
> + priv->beta_values[i] = 16;
memset32() ?
> + for (i = 0; i < 4; i++)
> + priv->ideality_value[i] = 18;
Ditto.
> + }
> +
> + /*
> + * Set default values in registers.
> + * APDD, RECD12 and RECD34 are active on 0.
> + */
> + val = FIELD_PREP(MCP9982_CFG_MSKAL, 1) | FIELD_PREP(MCP9982_CFG_RS, 1) |
> + FIELD_PREP(MCP9982_CFG_ATTHM, 1) |
> + FIELD_PREP(MCP9982_CFG_RECD12, !priv->recd12_enable) |
> + FIELD_PREP(MCP9982_CFG_RECD34, !priv->recd34_enable) |
> + FIELD_PREP(MCP9982_CFG_RANGE, 0) | FIELD_PREP(MCP9982_CFG_DA_ENA, 0) |
> + FIELD_PREP(MCP9982_CFG_APDD, !priv->apdd_enable);
> +
> + ret = regmap_write(priv->regmap, MCP9982_CFG_ADDR, val);
> + if (ret)
> + return ret;
> + priv->extended_temp_range = false;
> +
> + ret = regmap_write(priv->regmap, MCP9982_CONV_ADDR, 6);
> + if (ret)
> + return ret;
> + priv->sampl_idx = 6;
> +
> + ret = regmap_write(priv->regmap, MCP9982_HYS_ADDR, 10);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(priv->regmap, MCP9982_CONSEC_ALRT_ADDR, 112);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(priv->regmap, MCP9982_RUNNING_AVG_ADDR, 0);
> + if (ret)
> + return ret;
> +
> + ret = regmap_write(priv->regmap, MCP9982_HOTTEST_CFG_ADDR, 0);
> + if (ret)
> + return ret;
> +
> + /* Set beta compensation for channels 1 and 2 */
> + for (i = 0; i < 2; i++) {
ARRAY_SIZE()
> + ret = regmap_write(priv->regmap, MCP9982_EXT_BETA_CFG_ADDR(i),
> + priv->beta_values[i]);
> + if (ret)
> + return ret;
> + }
> + /* Set ideality factor for all external channels */
> + for (i = 0; i < 4; i++) {
Ditto.
> + ret = regmap_write(priv->regmap, MCP9982_EXT_IDEAL_ADDR(i),
> + priv->ideality_value[i]);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
...
> + priv->beta_values[0] = 16;
> + priv->beta_values[1] = 16;
memset32() ?
> + device_property_read_u32(dev, "microchip,beta1", &priv->beta_values[0]);
> + device_property_read_u32(dev, "microchip,beta2", &priv->beta_values[1]);
> + if (priv->beta_values[0] > 16 || priv->beta_values[1] > 16)
> + return -EINVAL;
...
> + if (priv->num_channels > device_nr_channels)
> + return dev_err_probe(dev, -EINVAL, "More channels than the chip supports\n");
Hmm... Perhaps -E2BIG?
...
> + priv->labels[0] = "internal diode";
> + iio_idx++;
> + device_for_each_child_node_scoped(dev, child) {
> + fwnode_property_read_u32(child, "reg", ®_nr);
> + if (!reg_nr || reg_nr >= device_nr_channels)
> + return dev_err_probe(dev, -EINVAL,
> + "The index of the channels does not match the chip\n");
> +
> + priv->ideality_value[reg_nr - 1] = 18;
> + if (fwnode_property_present(child, "microchip,ideality-factor")) {
> + fwnode_property_read_u32(child, "microchip,ideality-factor",
> + &priv->ideality_value[reg_nr - 1]);
> + if (priv->ideality_value[reg_nr - 1] > 63)
> + return dev_err_probe(dev, -EINVAL,
-EOVERFLOW?
> + "The ideality value is higher than maximum\n");
> + }
> +
> + fwnode_property_read_string(child, "label",
> + &priv->labels[reg_nr]);
> +
> + priv->iio_chan[iio_idx++] = MCP9982_CHAN(reg_nr, reg_nr,
> + MCP9982_INT_VALUE_ADDR(reg_nr));
> + }
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists