| 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: <20250530175351.000039cb@huawei.com>
Date: Fri, 30 May 2025 17:53:51 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.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, 29 May 2025 12:36:28 +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>
Hi Victor,
As often the case, the questions are mostly about the custom ABI.
Jonathan
> ---
> .../testing/sysfs-bus-iio-temperature-mcp9982 | 27 +
> MAINTAINERS | 7 +
> drivers/iio/temperature/Kconfig | 10 +
> drivers/iio/temperature/Makefile | 1 +
> drivers/iio/temperature/mcp9982.c | 866 ++++++++++++++++++
> 5 files changed, 911 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-bus-iio-temperature-mcp9982
> create mode 100644 drivers/iio/temperature/mcp9982.c
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-iio-temperature-mcp9982 b/Documentation/ABI/testing/sysfs-bus-iio-temperature-mcp9982
> new file mode 100644
> index 000000000000..c55e106e5863
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-iio-temperature-mcp9982
> @@ -0,0 +1,27 @@
> +What: /sys/bus/iio/devices/iio:deviceX/enable_extended_temp_range
> +KernelVersion: 6.17
> +Contact: linux-iio@...r.kernel.org
> +Description:
> + This attribute enables extended range of temperatures.
> + Value 1 uses the extended range, value 0 uses the default range.
As mentioned below, if this corresponds to -64 as the offset, can we expose
it as in_voltage_offset or similar?
> +
> +What: /sys/bus/iio/devices/iio:deviceX/in_beta1
> +KernelVersion: 6.17
> +Contact: linux-iio@...r.kernel.org
> +Description:
> + This attribute controls the value of beta correction
> + for channel 1.
Is this something that we'd normally expect to manually update? What is
it a characteristic of? If it is expected to the be related to the
diodes attached, that's a problem for firmware/dt, not sysfs interfaces.
> +
> +What: /sys/bus/iio/devices/iio:deviceX/in_beta2
> +KernelVersion: 6.17
> +Contact: linux-iio@...r.kernel.org
> +Description:
> + This attribute controls the value of beta correction
> + for channel 2.
> +
> +What: /sys/bus/iio/devices/iio:deviceX/in_beta_available
> +KernelVersion: 6.17
> +Contact: linux-iio@...r.kernel.org
> +Description:
> + This attribute displays the values that can be
> + set for beta correction.
Definitely mention the automode in these docs.
> diff --git a/drivers/iio/temperature/mcp9982.c b/drivers/iio/temperature/mcp9982.c
> new file mode 100644
> index 000000000000..89e966beffe6
> --- /dev/null
> +++ b/drivers/iio/temperature/mcp9982.c
> @@ -0,0 +1,866 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * IIO driver for MCP998X/33 and MCP998XD/33D Multichannel Automotive Temperature Monitor Family
> + *
> + * Copyright (C) 2025 Microchip Technology Inc. and its subsidiaries
> + *
> + * Author: Victor Duicu <victor.duicu@...rochip.com>
> + *
> + * Datasheet can be found here:
> + * https://ww1.microchip.com/downloads/aemDocuments/documents/MSLD/ProductDocuments/DataSheets/MCP998X-Family-Data-Sheet-DS20006827.pdf
> + */
> +
> +#include <linux/array_size.h>
> +#include <linux/bitfield.h>
> +#include <linux/bits.h>
> +#include <linux/device/devres.h>
> +#include <linux/dev_printk.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/irq.h>
Check if these are all needed. This one doesn't seem to be...
> +#include <linux/limits.h>
> +#include <linux/property.h>
> +#include <linux/regmap.h>
> +#include <linux/string.h>
> +#include <linux/unaligned.h>
> +#include <linux/units.h>
> +
> +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,
I'd use single space before =
Forcing alignment goes wrong far too often and doesn't really help with readability.
> +};
> +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;
> +
> + 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;
As that's the case, can we control this simply via standard ABI of IIO_CHAN_INFO_OFFSET and
provide the available as well. It is a little non obvious, but has the advantage of providing
the control you want with out the need for custom ABI.
> +
> + 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;
> + *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;
> +
> + 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))
It won't be greater than so == is appropriate.
> + 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]))
As above.
> + return -EINVAL;
> +
> + /*
> + * Filter register is coded with values:
> + *-0 for OFF
> + *-1 or 2 for level 1
> + *-3 for level 2
> + */
> + 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 you end up keeping this we have kstrtobool which tends to be more flexible
for an on/off parameter.
> + if (ret)
> + return -EINVAL;
> +
> + switch (val) {
> + case 0:
> + priv->extended_temp_range = false;
> + break;
> + case 1:
> + priv->extended_temp_range = true;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + 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;
> +
> + /* 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
> + 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;
> +
> + /* When APDD is enabled, betas are locked to autodetection */
> + if (priv->apdd_enable)
> + return -EINVAL;
> +
> + 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;
> + }
This interface is non obvious. Add a full description to the ABI docs.
> +
> + 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, " ");
> + }
> + strcat(buf, "Diode_Mode Auto\n");
As above. What is this?
> + return sysfs_emit(buf, buf);
> +}
> +
> +static int mcp9982_parse_of_config(struct iio_dev *indio_dev, struct device *dev,
> + int device_nr_channels)
> +{
> + struct mcp9982_priv *priv = iio_priv(indio_dev);
> + int reg_nr, iio_idx;
> +
> + /* APDD, RECD12 and RECD34 are active on 0 */
> + if (device_property_read_bool(dev, "microchip,apdd-state"))
> + priv->apdd_enable = true;
> + else
> + priv->apdd_enable = false;
priv->appd_enable = device_property_read_bool();
> +
> + if (device_property_read_bool(dev, "microchip,recd12"))
> + priv->recd12_enable = true;
> + else
> + priv->recd12_enable = false;
> +
> + if (device_property_read_bool(dev, "microchip,recd34"))
> + priv->recd34_enable = true;
> + else
> + priv->recd34_enable = false;
> +
> + priv->num_channels = device_get_child_node_count(dev) + 1;
> +
> + if (priv->num_channels > device_nr_channels)
> + return dev_err_probe(dev, -EINVAL, "More channels than the chip supports\n");
> +
> + priv->iio_chan = devm_kcalloc(dev, priv->num_channels, sizeof(*priv->iio_chan), GFP_KERNEL);
> + if (!priv->iio_chan)
> + return -ENOMEM;
> +
> + priv->iio_chan[0] = (struct iio_chan_spec)MCP9982_CHAN(0, 0, MCP9982_INT_VALUE_ADDR(0));
as below.
> +
> + 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");
> +
> + 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,
> + "The ideality value is higher than maximum\n");
> + } else {
> + /* Set default value */
> + priv->ideality_value[reg_nr - 1] = 18;
> + }
Set default and rely on no side effects if the read fails + that default must
be a valid choice.
priv->ideality_value[reg_nr - 1] = 18;
fwnode_property_read_u32(child, "microchip,ideality-factor",
&priv->ideality_value[reg_nr - 1]);
if (priv->ideality_vavlue[reg_nr - 1] > 63)
return dev_err_probe();
> +
> + fwnode_property_read_string(child, "label",
> + (const char **)&priv->labels[reg_nr]);
I'm curious why that cast is needed. Should the string in the priv be const?
Do we perhaps have an issue with that elsewhere?
> +
> + priv->iio_chan[iio_idx++] = (struct iio_chan_spec)MCP9982_CHAN(reg_nr, reg_nr,
> + MCP9982_INT_VALUE_ADDR(reg_nr));
Seems very likely that the (struct iio_chan_spec) should be in the macro definition.
> + }
> +
> + return 0;
> +}
> +
> +static int mcp9982_probe(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + struct mcp9982_priv *priv;
> + struct iio_dev *indio_dev;
> + const struct mcp9982_features *chip;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(dev, sizeof(*priv));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + priv = iio_priv(indio_dev);
> + i2c_set_clientdata(client, indio_dev);
Is this used? If not don't set it.
> +
> + priv->regmap = devm_regmap_init_i2c(client, &mcp9982_regmap_config);
> + if (IS_ERR(priv->regmap))
> + return dev_err_probe(dev, PTR_ERR(priv->regmap),
> + "Cannot initialize register map\n");
> +
> + ret = devm_mutex_init(dev, &priv->lock);
> + if (ret)
> + return ret;
> +
> + chip = i2c_get_match_data(client);
> + if (!chip)
> + return -EINVAL;
> +
> + ret = mcp9982_parse_of_config(indio_dev, &client->dev, chip->phys_channels);
> + if (ret)
> + return dev_err_probe(dev, ret, "Parameter parsing error\n");
> +
> + ret = mcp9982_init(priv);
> + if (ret)
> + return dev_err_probe(dev, ret, "Cannot initialize device\n");
> +
> + indio_dev->name = chip->name;
> + indio_dev->info = &mcp9982_info;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = priv->iio_chan;
> + indio_dev->num_channels = priv->num_channels;
> +
> + ret = devm_iio_device_register(dev, indio_dev);
> + if (ret)
> + return dev_err_probe(dev, ret, "Cannot register IIO device\n");
> +
> + mcp9982_calc_all_3db_values();
Seems likely you want to do this before exposing the sysfs interfaces?
If not blank line before this return.
> + return 0;
> +}
Powered by blists - more mailing lists