| 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: <20250812165120.00004f37@huawei.com>
Date: Tue, 12 Aug 2025 16:51:20 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Ben Collins <bcollins@...nel.org>
CC: <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 Tue, 12 Aug 2025 09:08:30 -0400
Ben Collins <bcollins@...nel.org> 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
>
> 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>
Hi
I tried not to overlap too much with David's review.
A few additional things inline.
Jonathan
> 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
> +
> +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);
> +
No blank line here. Keep error check associated with the call.
> + 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:
If this is something we specify in DT (which I think is valid) then
we probably should not allow override from userspace.
We might want a read only parameter though as type of thermocouple is
probably something that is useful to be able to read back.
> + 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;
return mcp9600_config(data);
Assuming that never returns > 0.
> +
> + break;
> +
> default:
> return -EINVAL;
> }
> +
> + return 0;
Might as well return rather than break above.
> }
> 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");
Too noisy. It is fine to over ride the DT compatible if we match a known ID, but if
not paper over the missmatch so that in future a new chip can be supported via
a fall back compatible entry without a kernel upgrade. dev_info on an unknown
device is fine though.
> + break;
> + case MCP9600_DEVICE_ID_MCP9601:
> + dev_info(&client->dev, "Identified as mcp9601");
> + break;
> +
> + 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) {
Likewise, must have a default value if it wasn't there from start so with
that in mind can use a similar pattern to the below suggestion.
> + dev_warn(&client->dev,
> + "Missing thermocouple-type property, using Type-K\n");
> + 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");
> + 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;
If we were keeping this a common pattern for this sort of property (which can't
be required as wasn't there from the start) is
make data->filter a u32 if it isn't already.
data->filter = 0;
device_property_read_u32(&client->dev, "filter-level", &data->filter);
if (data->filter > 7) {
dev_warn(&...
data->filter = 0;
}
That is rely on no side effects in the property read if it returns an error,
unsigned property not less than 0 and that 0 is an allowed value.
However as David said unlikely to be something that belongs in DT.
> + } 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