Re: [PATCH] mcp9600: Add support for mcp9601 and sensor config

[Jonathan Cameron <Jonathan.Cameron@huawei.com>]

On Tue, 12 Aug 2025 09:08:30 -0400
Ben Collins <bcollins@kernel.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@kernel.org>
> Cc: Andrew Hepp <andrew.hepp@ahepp.dev>
> Cc: devicetree@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-iio@vger.kernel.org
> Cc: "Nuno Sá" <nuno.sa@analog.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;