| 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: <2214d559-5a5f-40e5-aa46-298ac2eaa9c2@baylibre.com>
Date: Tue, 12 Aug 2025 10:34:06 -0500
From: David Lechner <dlechner@...libre.com>
To: linux-iio@...r.kernel.org, Andrew Hepp <andrew.hepp@...pp.dev>,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
Nuno Sá <nuno.sa@...log.com>
Subject: Re: [PATCH] mcp9600: Add support for mcp9601 and sensor config
On 8/12/25 8:08 AM, Ben Collins wrote:
> The mcp9600 dt binding doc claims to support thermocouple-type but
> I don't see where this is implemented.
>
> - Add support to detect mcp9601 device type
> - Add support to use thermocouple-type dt prop
> - Add thrermocouple iio info to get/set this from sysfs
> - Add filter-level dt prop to set the filtering level of the chip
> - Update dt binding docs
This is too much for one patch. Split the thermocouple type and
filter level into separate patches.
>
> Signed-off-by: Ben Collins <bcollins@...nel.org>
> Cc: Andrew Hepp <andrew.hepp@...pp.dev>
> Cc: devicetree@...r.kernel.org
> Cc: linux-kernel@...r.kernel.org
> Cc: linux-iio@...r.kernel.org
> Cc: "Nuno Sá" <nuno.sa@...log.com>
> ---
The dependency on your other patch should be mentioned here.
Or just include the other patch in the series too.
> .../iio/temperature/microchip,mcp9600.yaml | 9 +
> drivers/iio/temperature/mcp9600.c | 185 ++++++++++++++++--
> 2 files changed, 181 insertions(+), 13 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml
> index d2cafa38a5442..0ee0259471c6a 100644
> --- a/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml
> +++ b/Documentation/devicetree/bindings/iio/temperature/microchip,mcp9600.yaml
> @@ -42,6 +42,14 @@ properties:
> Use defines in dt-bindings/iio/temperature/thermocouple.h.
> Supported types are B, E, J, K, N, R, S, T.
>
> + filter-level:
> + $ref: /schemas/types.yaml#/definitions/uint32
> + description:
> + Level of chip filtering to use. 0 means filtering is disabled.
> + See reference manual 5.2.2 - THERMOCOUPLE SENSOR CONFIGURATION
> + REGISTER, FIGURE 5-6 for Filter Step Response graph. Supported
> + values are in the range of 0-7.
> +
Devicetree binding changes need to be in a separate patch and CC the
relevant maintainers.
However, this looks like something that doesn't belong in a devicetree.
Unless you can justify why the filtering level depends on how the
system is wired up, this is more likely something that should be
done at runtime.
I only had a very quick look a the datasheet, but I think using
IIO_CHAN_INFO_LOW_PASS_FILTER_3DB_FREQUENCY to control the filter
range would make sense.
> vdd-supply: true
>
> required:
> @@ -65,6 +73,7 @@ examples:
> interrupts = <25 IRQ_TYPE_EDGE_RISING>;
> interrupt-names = "open-circuit";
> thermocouple-type = <THERMOCOUPLE_TYPE_K>;
> + filter-level = <1>;
> vdd-supply = <&vdd>;
> };
> };
> diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
> index 6e9108d5cf75f..c1fe1e530786c 100644
> --- a/drivers/iio/temperature/mcp9600.c
> +++ b/drivers/iio/temperature/mcp9600.c
> @@ -18,15 +18,23 @@
> #include <linux/minmax.h>
> #include <linux/mod_devicetable.h>
> #include <linux/module.h>
> +#include <dt-bindings/iio/temperature/thermocouple.h>
>
> #include <linux/iio/events.h>
> #include <linux/iio/iio.h>
>
> /* MCP9600 registers */
> -#define MCP9600_HOT_JUNCTION 0x0
> -#define MCP9600_COLD_JUNCTION 0x2
> +#define MCP9600_HOT_JUNCTION 0x0
> +#define MCP9600_COLD_JUNCTION 0x2
Whitespace changes should also be in a separate patch to avoid
distraction from the actual changes.
> #define MCP9600_STATUS 0x4
> #define MCP9600_STATUS_ALERT(x) BIT(x)
> +#define MCP9600_STATUS_OC_IR BIT(4)
> +#define MCP9601_STATUS_SC BIT(5)
> +#define MCP9600_SENSOR_CFG 0x5
> +#define MCP9600_SENSOR_TYPE_MASK GENMASK(6, 4)
> +#define MCP9600_SENSOR_TYPE(x) ((x << 4) & MCP9600_SENSOR_TYPE_MASK)
We typically avoid making macros like this and just use
FIELD_PREP() at the call site.
> +#define MCP9600_FILTER_MASK GENMASK(2, 0)
> +#define MCP9600_FILTER(x) ((x << 0) & MCP9600_FILTER_MASK)
> #define MCP9600_ALERT_CFG1 0x8
> #define MCP9600_ALERT_CFG(x) (MCP9600_ALERT_CFG1 + (x - 1))
> #define MCP9600_ALERT_CFG_ENABLE BIT(0)
> @@ -38,10 +46,11 @@
> #define MCP9600_ALERT_LIMIT1 0x10
> #define MCP9600_ALERT_LIMIT(x) (MCP9600_ALERT_LIMIT1 + (x - 1))
> #define MCP9600_ALERT_LIMIT_MASK GENMASK(15, 2)
> -#define MCP9600_DEVICE_ID 0x20
> +#define MCP9600_DEVICE_ID 0x20
>
> /* MCP9600 device id value */
> -#define MCP9600_DEVICE_ID_MCP9600 0x40
> +#define MCP9600_DEVICE_ID_MCP9600 0x40
> +#define MCP9600_DEVICE_ID_MCP9601 0x41
>
> #define MCP9600_ALERT_COUNT 4
>
> @@ -58,6 +67,21 @@ enum mcp9600_alert {
> MCP9600_ALERT4
> };
>
> +static const unsigned int mcp9600_type_map[] = {
> + [THERMOCOUPLE_TYPE_K] = 0,
> + [THERMOCOUPLE_TYPE_J] = 1,
> + [THERMOCOUPLE_TYPE_T] = 2,
> + [THERMOCOUPLE_TYPE_N] = 3,
> + [THERMOCOUPLE_TYPE_S] = 4,
> + [THERMOCOUPLE_TYPE_E] = 5,
> + [THERMOCOUPLE_TYPE_B] = 6,
> + [THERMOCOUPLE_TYPE_R] = 7,
> +};
> +
> +static const int mcp9600_tc_types[] = {
> + 'B', 'E', 'J', 'K', 'N', 'R', 'S', 'T'
> +};
> +
> static const char * const mcp9600_alert_name[MCP9600_ALERT_COUNT] = {
> [MCP9600_ALERT1] = "alert1",
> [MCP9600_ALERT2] = "alert2",
> @@ -87,8 +111,12 @@ static const struct iio_event_spec mcp9600_events[] = {
> { \
> .type = IIO_TEMP, \
> .address = MCP9600_HOT_JUNCTION, \
> - .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> - BIT(IIO_CHAN_INFO_SCALE), \
> + .info_mask_separate = \
> + BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_THERMOCOUPLE_TYPE) | \
> + BIT(IIO_CHAN_INFO_SCALE), \
> + .info_mask_separate_available = \
> + BIT(IIO_CHAN_INFO_THERMOCOUPLE_TYPE), \
Since the type is set the the devicetree and can't be changed, there doesn't
seems much sense in listing the available types.
> .event_spec = &mcp9600_events[hj_ev_spec_off], \
> .num_event_specs = hj_num_ev, \
> }, \
> @@ -125,6 +153,9 @@ static const struct iio_chan_spec mcp9600_channels[][2] = {
>
> struct mcp9600_data {
> struct i2c_client *client;
> + unsigned char dev_id;
This isn't used outside of the probe function, so doesn't need to be here.
> + u32 thermocouple_type;
> + u32 filter;
filter_level
> };
>
> static int mcp9600_read(struct mcp9600_data *data,
> @@ -155,13 +186,82 @@ static int mcp9600_read_raw(struct iio_dev *indio_dev,
> if (ret)
> return ret;
> return IIO_VAL_INT;
> +
> case IIO_CHAN_INFO_SCALE:
> *val = 62;
> *val2 = 500000;
> return IIO_VAL_INT_PLUS_MICRO;
> +
> + case IIO_CHAN_INFO_THERMOCOUPLE_TYPE:
> + *val = mcp9600_tc_types[data->thermocouple_type];
> + return IIO_VAL_CHAR;
> +
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int mcp9600_write_raw_get_fmt(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + long mask)
> +{
> + switch (mask) {
> + case IIO_CHAN_INFO_THERMOCOUPLE_TYPE:
> + return IIO_VAL_CHAR;
> + default:
> + return -EINVAL;
> + }
> +}
> +
> +static int mcp9600_config(struct mcp9600_data *data)
> +{
> + struct i2c_client *client = data->client;
> + int ret, cfg;
> +
> + cfg = MCP9600_SENSOR_TYPE(mcp9600_type_map[data->thermocouple_type]) |
> + MCP9600_FILTER(data->filter);
> +
> + ret = i2c_smbus_write_byte_data(client, MCP9600_SENSOR_CFG, cfg);
> +
> + if (ret < 0) {
> + dev_err(&client->dev, "Failed to set sensor configuration\n");
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int mcp9600_write_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + int val, int val2, long mask)
> +{
> + struct mcp9600_data *data = iio_priv(indio_dev);
> + int tc_type = -1;
> + int i, ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_THERMOCOUPLE_TYPE:
Why would we want to write over what was set in the devicetree?
> + for (i = 0; i < ARRAY_SIZE(mcp9600_tc_types); i++) {
> + if (mcp9600_tc_types[i] == toupper(val)) {
> + tc_type = i;
> + break;
> + }
> + }
> + if (tc_type < 0)
> + return -EINVAL;
> +
> + data->thermocouple_type = tc_type;
> + ret = mcp9600_config(data);
> + if (ret < 0)
> + return ret;
> +
> + break;
return 0; here
> +
> default:
> return -EINVAL;
> }
> +
> + return 0;
rather than here
> }
>
> static int mcp9600_get_alert_index(int channel2, enum iio_event_direction dir)
> @@ -299,12 +399,32 @@ static int mcp9600_write_thresh(struct iio_dev *indio_dev,
> }
> }
>
> +static int mcp9600_read_avail(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *chan,
> + const int **vals, int *type, int *length,
> + long mask)
> +{
> + switch (mask) {
> + case IIO_CHAN_INFO_THERMOCOUPLE_TYPE:
> + *vals = mcp9600_tc_types;
> + *type = IIO_VAL_CHAR;
> + *length = ARRAY_SIZE(mcp9600_tc_types);
> + return IIO_AVAIL_LIST;
> +
> + default:
> + return -EINVAL;
> + }
> +}
> +
> static const struct iio_info mcp9600_info = {
> .read_raw = mcp9600_read_raw,
> + .write_raw = mcp9600_write_raw,
> + .write_raw_get_fmt = mcp9600_write_raw_get_fmt,
> .read_event_config = mcp9600_read_event_config,
> .write_event_config = mcp9600_write_event_config,
> .read_event_value = mcp9600_read_thresh,
> .write_event_value = mcp9600_write_thresh,
> + .read_avail = mcp9600_read_avail,
> };
>
> static irqreturn_t mcp9600_alert_handler(void *private,
> @@ -418,26 +538,65 @@ static int mcp9600_probe(struct i2c_client *client)
> {
> struct iio_dev *indio_dev;
> struct mcp9600_data *data;
> - int ret, ch_sel;
> + int ch_sel, dev_id, ret;
>
> - ret = i2c_smbus_read_byte_data(client, MCP9600_DEVICE_ID);
> - if (ret < 0)
> - return dev_err_probe(&client->dev, ret, "Failed to read device ID\n");
> - if (ret != MCP9600_DEVICE_ID_MCP9600)
> - dev_warn(&client->dev, "Expected ID %x, got %x\n",
> - MCP9600_DEVICE_ID_MCP9600, ret);
> + dev_id = i2c_smbus_read_byte_data(client, MCP9600_DEVICE_ID);
> + if (dev_id < 0)
> + return dev_err_probe(&client->dev, dev_id, "Failed to read device ID\n");
> +
> + switch (dev_id) {
> + case MCP9600_DEVICE_ID_MCP9600:
> + dev_info(&client->dev, "Identified as mcp9600");
> + break;
> + case MCP9600_DEVICE_ID_MCP9601:
> + dev_info(&client->dev, "Identified as mcp9601");
> + break;
> +
Experience has show that depending on ICs to report the correct device ID
is fragile. Instead, we should depend on the compatible string. Printing
this for debug purposes is acceptable though, but should be a separate
patch. Just don't fail if it is an unexpected value.
> + default:
> + return dev_err_probe(&client->dev, -EINVAL, "Unknown device ID: %x\n",
> + dev_id);
> + }
>
> indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> if (!indio_dev)
> return -ENOMEM;
>
> data = iio_priv(indio_dev);
> +
> + ret = device_property_read_u32(&client->dev, "thermocouple-type",
> + &data->thermocouple_type);
> + if (ret) {
> + dev_warn(&client->dev,
> + "Missing thermocouple-type property, using Type-K\n");
The devicetree bindings should have a default listed. So this should not be
a warning, but rather the expected default.
> + data->thermocouple_type = THERMOCOUPLE_TYPE_K;
> + } else if (data->thermocouple_type < 0 || data->thermocouple_type >=
> + ARRAY_SIZE(mcp9600_type_map)) {
> + dev_warn(&client->dev,
> + "Invalid thermocouple-type property, using Type-K\n");
Should fail with error if the devicetree gives an invalid value.
> + data->thermocouple_type = THERMOCOUPLE_TYPE_K;
> + }
> +
> + ret = device_property_read_u32(&client->dev, "filter-level",
> + &data->filter);
> + if (ret) {
> + dev_warn(&client->dev,
> + "Missing filter-level property, using 0\n");
> + data->filter = 0;
> + } else if (data->filter < 0 || data->filter > 7) {
> + dev_warn(&client->dev,
> + "Invalid filter-level property, using 0\n");
> + data->filter = 0;
> + }
> +
> + data->dev_id = dev_id;
> data->client = client;
>
> ch_sel = mcp9600_probe_alerts(indio_dev);
> if (ch_sel < 0)
> return ch_sel;
>
> + mcp9600_config(data);
> +
> indio_dev->info = &mcp9600_info;
> indio_dev->name = "mcp9600";
> indio_dev->modes = INDIO_DIRECT_MODE;
Powered by blists - more mailing lists