| 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: <20250830203446.5c164f74@jic23-huawei>
Date: Sat, 30 Aug 2025 20:34:46 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: <victor.duicu@...rochip.com>
Cc: <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 v4 2/2] iio: temperature: add support for MCP998X
On Fri, 29 Aug 2025 17:34:47 +0300
<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.
>
> Signed-off-by: Victor Duicu <victor.duicu@...rochip.com>
Tried to avoid duplication with other reviewers but probably failed!
Jonathan
> diff --git a/drivers/iio/temperature/mcp9982.c b/drivers/iio/temperature/mcp9982.c
> new file mode 100644
> index 000000000000..2f0b9c4674fb
> --- /dev/null
> +++ b/drivers/iio/temperature/mcp9982.c
> +
> +static const struct regmap_config mcp9982_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .rd_table = &mcp9982_regmap_rd_table,
> + .wr_table = &mcp9982_regmap_wr_table,
> + .volatile_reg = mcp9982_is_volatile_reg,
> +};
> +
> +/**
> + * struct mcp9992_priv - information about chip parameters
> + * @regmap: device register map
> + * @chip pointer to structure holding chip features
> + * @lock synchronize access to driver's state members
> + * @iio_chan specifications of channels
> + * @labels labels of the channels
> + * @ideality_value ideality factor value for each external channel
> + * @sampl_idx index representing the current sampling frequency
> + * @time_limit time when it is safe to read
> + * @recd34_enable state of REC on channels 3 and 4
Spell out REC fully. It's not a well enough known term anyone reading that comment
might be expected to know what it means.
> + * @recd12_enable state of REC on channels 1 and 2
> + * @apdd_enable state of anti-parallel diode mode
> + * @run_state chip is in run state, otherwise is in standby state
> + * @wait_before_read whether we need to wait a delay before reading a new value
> + * @num_channels number of active physical channels
> + */
> +struct mcp9982_priv {
> + struct regmap *regmap;
> + const struct mcp9982_features *chip;
> + /*
> + * 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;
> + unsigned long time_limit;
> + bool recd34_enable;
> + bool recd12_enable;
> + bool apdd_enable;
> + bool run_state;
> + bool wait_before_read;
> + u8 num_channels;
> +};
> +static int mcp9982_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan, int val,
> + int val2, long mask)
> +{
> + unsigned int i, start, previous_sampl_idx;
> + struct mcp9982_priv *priv = iio_priv(indio_dev);
> + int ret;
> + unsigned long new_time_limit;
> +
> + start = 0;
> + guard(mutex)(&priv->lock);
> + switch (mask) {
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + previous_sampl_idx = priv->sampl_idx;
> + /*
> + * For MCP998XD and MCP9933D sampling frequency can't
> + * be set lower than 1.
wrap at 80 chars, not 70ish.
> + */
> + if (priv->chip->hw_thermal_shutdown)
> + 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;
> +
> + /*
> + * in Run mode, when changing the frequency, wait a delay based
In Run mode,
> + * on the previous value to ensure the new value becomes active
> + */
> + if (priv->run_state) {
> + new_time_limit = jiffies +
> + msecs_to_jiffies(mcp9982_delay_ms[previous_sampl_idx]);
> + if (time_after(new_time_limit, priv->time_limit)) {
> + priv->time_limit = new_time_limit;
> + priv->wait_before_read = true;
> + }
> + return 0;
> + }
> +
> + break;
>
> +static int mcp9982_init(struct mcp9982_priv *priv)
> +{
> + int ret;
> + unsigned int i;
> + u8 val;
> +
> + /* Chips 82/83 and 82D/83D do not support anti-parallel diode mode */
> + if (!priv->chip->allow_apdd)
> + priv->apdd_enable = 0;
> +
> + /*
> + * Chips with "D" work in Run state and those without work
> + * in Standby state
> + */
> + if (priv->chip->hw_thermal_shutdown)
> + priv->run_state = 1;
> + else
> + priv->run_state = 0;
runstate = priv->chip->hw_thermal_shutdown;
> +
> + /*
> + * For chips with "D" in the name set the below parameters to default to
> + * ensure that hardware shutdown feature can't be overridden.
> + */
> + if (priv->chip->hw_thermal_shutdown) {
> + priv->recd12_enable = true;
> + priv->recd34_enable = true;
> + }
> +
> + /*
> + * Set default values in registers. APDD, RECD12 and RECD34 are active
> + * on 0.
Probably add a line break between those sentences. I think the first one
is applying to all these writes, whereas second sentence is just about this one.
> + */
> + val = FIELD_PREP(MCP9982_CFG_MSKAL, 1) |
> + FIELD_PREP(MCP9982_CFG_RS, !priv->run_state) |
> + 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, 1) | 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;
> +
> + 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 auto-detection beta compensation for channels 1 and 2 */
> + for (i = 0; i < 2; i++) {
> + ret = regmap_write(priv->regmap, MCP9982_EXT_BETA_CFG_ADDR(i),
> + MCP9982_BETA_AUTODETECT);
> + if (ret)
> + return ret;
> + }
> + /* Set ideality factor for all external channels */
> + for (i = 0; i < ARRAY_SIZE(priv->ideality_value); i++) {
> + ret = regmap_write(priv->regmap, MCP9982_EXT_IDEAL_ADDR(i),
> + priv->ideality_value[i]);
> + if (ret)
> + return ret;
> + }
> +
> + priv->wait_before_read = false;
> + priv->time_limit = jiffies;
> +
> + return 0;
> +}
> +
> +static int mcp9982_parse_of_config(struct iio_dev *indio_dev, struct device *dev,
> + int device_nr_channels)
> +{
> + unsigned int reg_nr, iio_idx;
> + struct mcp9982_priv *priv = iio_priv(indio_dev);
> +
> + priv->apdd_enable = device_property_read_bool(dev,
> + "microchip,enable-anti-parallel");
I'd suggestion these are more readable as.
priv->apdd_enable =
device_property_read_bool(dev, "microchip,enable-anti-parallel");
> +
> + priv->recd12_enable = device_property_read_bool(dev,
> + "microchip,parasitic-res-on-channel1-2");
> +
> + priv->recd34_enable = device_property_read_bool(dev,
> + "microchip,parasitic-res-on-channel3-4");
> +
> + priv->num_channels = device_get_child_node_count(dev) + 1;
> +
> + if (priv->num_channels > device_nr_channels)
> + return dev_err_probe(dev, -E2BIG,
> + "More channels than the chip supports\n");
> +
> + priv->iio_chan = devm_kcalloc(dev, priv->num_channels,
> + sizeof(*priv->iio_chan), GFP_KERNEL);
Seems channels can't be more than 6(?) I'd just directly embed a large enough array
in priv so no allocation here necessary.
> + if (!priv->iio_chan)
> + return -ENOMEM;
> +
> + priv->iio_chan[0] = MCP9982_CHAN(0, 0, MCP9982_INT_VALUE_ADDR(0));
> +
> + 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, -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));
> + }
> +
> + return 0;
> +}
> +
> +static int mcp9982_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
...
> + ret = mcp9982_parse_of_config(indio_dev, &client->dev, chip->phys_channels);
parse_fw_config as its not DT specific (which is excellent!)
Also use dev.
> + if (ret)
> + return dev_err_probe(dev, ret, "Parameter parsing error\n");
Powered by blists - more mailing lists