The current driver works with mcp9601, but emits a warning because it
does not recognize the chip id.
MCP9601 is a superset of MCP9600. The drivers works without changes
on this chipset.
However, the 9601 chip supports open/closed-circuit detection if wired
properly, so we'll need to be able to differentiate between them.
Moved "struct mcp9600_data" up in the file since a later patch will
need it and chip_info before the declerations.
Signed-off-by: Ben Collins <bcollins@watter.com>
Reviewed-by: David Lechner <dlechner@baylibre.com>
---
drivers/iio/temperature/Kconfig | 8 +++--
drivers/iio/temperature/mcp9600.c | 63 ++++++++++++++++++++++++++++++---------
2 files changed, 54 insertions(+), 17 deletions(-)
diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
index 1244d8e17d50..9328b2250ace 100644
--- a/drivers/iio/temperature/Kconfig
+++ b/drivers/iio/temperature/Kconfig
@@ -173,11 +173,13 @@ config MAX31865
will be called max31865.
config MCP9600
- tristate "MCP9600 thermocouple EMF converter"
+ tristate "MCP9600 and similar thermocouple EMF converters"
depends on I2C
help
- If you say yes here you get support for MCP9600
- thermocouple EMF converter connected via I2C.
+ If you say yes here you get support for...
+ - MCP9600
+ - MCP9601
+ ...thermocouple EMF converters connected via I2C.
This driver can also be built as a module. If so, the module
will be called mcp9600.
diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
index 40906bb200ec..4654b3aaaf2a 100644
--- a/drivers/iio/temperature/mcp9600.c
+++ b/drivers/iio/temperature/mcp9600.c
@@ -42,6 +42,7 @@
/* MCP9600 device id value */
#define MCP9600_DEVICE_ID_MCP9600 0x40
+#define MCP9600_DEVICE_ID_MCP9601 0x41
#define MCP9600_ALERT_COUNT 4
@@ -82,6 +83,15 @@ static const struct iio_event_spec mcp9600_events[] = {
},
};
+struct mcp_chip_info {
+ u8 chip_id;
+ const char *chip_name;
+};
+
+struct mcp9600_data {
+ struct i2c_client *client;
+};
+
#define MCP9600_CHANNELS(hj_num_ev, hj_ev_spec_off, cj_num_ev, cj_ev_spec_off) \
{ \
{ \
@@ -123,10 +133,6 @@ static const struct iio_chan_spec mcp9600_channels[][2] = {
MCP9600_CHANNELS(2, 0, 2, 0), /* Alerts: 1 2 3 4 */
};
-struct mcp9600_data {
- struct i2c_client *client;
-};
-
static int mcp9600_read(struct mcp9600_data *data,
struct iio_chan_spec const *chan, int *val)
{
@@ -416,16 +422,33 @@ static int mcp9600_probe_alerts(struct iio_dev *indio_dev)
static int mcp9600_probe(struct i2c_client *client)
{
+ const struct mcp_chip_info *chip_info = i2c_get_match_data(client);
struct iio_dev *indio_dev;
struct mcp9600_data *data;
- int ret, ch_sel;
+ int ch_sel, dev_id, ret;
+
+ if (!chip_info)
+ return dev_err_probe(&client->dev, -EINVAL,
+ "No chip-info found for device\n");
+
+ 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:
+ case MCP9600_DEVICE_ID_MCP9601:
+ if (dev_id != chip_info->chip_id)
+ dev_warn(&client->dev,
+ "Expected id %02x, but device responded with %02x\n",
+ chip_info->chip_id, dev_id);
+ break;
- 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);
+ default:
+ dev_warn(&client->dev, "Unknown id %x, using %x\n", dev_id,
+ chip_info->chip_id);
+ }
indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
if (!indio_dev)
@@ -439,7 +462,7 @@ static int mcp9600_probe(struct i2c_client *client)
return ch_sel;
indio_dev->info = &mcp9600_info;
- indio_dev->name = "mcp9600";
+ indio_dev->name = chip_info->chip_name;
indio_dev->modes = INDIO_DIRECT_MODE;
indio_dev->channels = mcp9600_channels[ch_sel];
indio_dev->num_channels = ARRAY_SIZE(mcp9600_channels[ch_sel]);
@@ -447,14 +470,26 @@ static int mcp9600_probe(struct i2c_client *client)
return devm_iio_device_register(&client->dev, indio_dev);
}
+static const struct mcp_chip_info mcp9600_chip_info = {
+ .chip_id = MCP9600_DEVICE_ID_MCP9600,
+ .chip_name = "mcp9600",
+};
+
+static const struct mcp_chip_info mcp9601_chip_info = {
+ .chip_id = MCP9600_DEVICE_ID_MCP9601,
+ .chip_name = "mcp9601",
+};
+
static const struct i2c_device_id mcp9600_id[] = {
- { "mcp9600" },
+ { "mcp9600", .driver_data = (kernel_ulong_t)&mcp9600_chip_info },
+ { "mcp9601", .driver_data = (kernel_ulong_t)&mcp9601_chip_info },
{ }
};
MODULE_DEVICE_TABLE(i2c, mcp9600_id);
static const struct of_device_id mcp9600_of_match[] = {
- { .compatible = "microchip,mcp9600" },
+ { .compatible = "microchip,mcp9600", .data = &mcp9600_chip_info },
+ { .compatible = "microchip,mcp9601", .data = &mcp9601_chip_info },
{ }
};
MODULE_DEVICE_TABLE(of, mcp9600_of_match);
--
2.39.5On Wed, Aug 20, 2025 at 2:45 AM Ben Collins <bcollins@watter.com> wrote:
>
> The current driver works with mcp9601, but emits a warning because it
> does not recognize the chip id.
>
> MCP9601 is a superset of MCP9600. The drivers works without changes
> on this chipset.
>
> However, the 9601 chip supports open/closed-circuit detection if wired
> properly, so we'll need to be able to differentiate between them.
>
> Moved "struct mcp9600_data" up in the file since a later patch will
> need it and chip_info before the declerations.
declarations
...
> +struct mcp9600_data {
> + struct i2c_client *client;
> +};
> +
> #define MCP9600_CHANNELS(hj_num_ev, hj_ev_spec_off, cj_num_ev, cj_ev_spec_off) \
> { \
> { \
> @@ -123,10 +133,6 @@ static const struct iio_chan_spec mcp9600_channels[][2] = {
> MCP9600_CHANNELS(2, 0, 2, 0), /* Alerts: 1 2 3 4 */
> };
>
> -struct mcp9600_data {
> - struct i2c_client *client;
> -};
> -
It's not obvious why this piece of change is needed. AFAICS it's a stray change.
...
> static int mcp9600_probe(struct i2c_client *client)
> {
> + const struct mcp_chip_info *chip_info = i2c_get_match_data(client);
> struct iio_dev *indio_dev;
> struct mcp9600_data *data;
> - int ret, ch_sel;
> + int ch_sel, dev_id, ret;
It's hard to maintain and prone to subtle errors if we split
assignment and check, so please move assignment here.
chip_info = i2c_get_match_data(client);
> + if (!chip_info)
> + return dev_err_probe(&client->dev, -EINVAL,
In such cases we usually use ENODEV.
> + "No chip-info found for device\n");
...
> + return dev_err_probe(&client->dev, dev_id,
> + "Failed to read device ID\n");
With
struct device *dev = &client->dev;
at the top of the function this and other statements become neater and
easier to follow. In particular, I believe this one may become a one
liner after the change.
--
With Best Regards,
Andy Shevchenko
> On Aug 20, 2025, at 6:07 AM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>
> On Wed, Aug 20, 2025 at 2:45 AM Ben Collins <bcollins@watter.com> wrote:
>>
>> The current driver works with mcp9601, but emits a warning because it
>> does not recognize the chip id.
>>
>> MCP9601 is a superset of MCP9600. The drivers works without changes
>> on this chipset.
>>
>> However, the 9601 chip supports open/closed-circuit detection if wired
>> properly, so we'll need to be able to differentiate between them.
>>
>> Moved "struct mcp9600_data" up in the file since a later patch will
>> need it and chip_info before the declerations.
>
> declarations
>
> ...
>
>> +struct mcp9600_data {
>> + struct i2c_client *client;
>> +};
>> +
>> #define MCP9600_CHANNELS(hj_num_ev, hj_ev_spec_off, cj_num_ev, cj_ev_spec_off) \
>> { \
>> { \
>> @@ -123,10 +133,6 @@ static const struct iio_chan_spec mcp9600_channels[][2] = {
>> MCP9600_CHANNELS(2, 0, 2, 0), /* Alerts: 1 2 3 4 */
>> };
>>
>> -struct mcp9600_data {
>> - struct i2c_client *client;
>> -};
>> -
>
> It's not obvious why this piece of change is needed. AFAICS it's a stray change.
The explanation is in the changelog above. A follow up patch needs both struct
declarations to be where I added one and moved mcp9600_data to. It’s just ordering
so I don’t later have to forward declare new functions for filter_type, which make
use of these structs, but need to be in the iio_chan_spec mcp9600_channels[]
declaration.
I guess I could move mcp9600_data in that series, but I had this in here before
I split that series out, and it seemed simple enough to leave in.
On Wed, Aug 20, 2025 at 07:11:08AM -0400, Ben Collins wrote:
> > On Aug 20, 2025, at 6:07 AM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
> > On Wed, Aug 20, 2025 at 2:45 AM Ben Collins <bcollins@watter.com> wrote:
...
> >> +struct mcp9600_data {
> >> + struct i2c_client *client;
> >> +};
> >> +
> >> #define MCP9600_CHANNELS(hj_num_ev, hj_ev_spec_off, cj_num_ev, cj_ev_spec_off) \
> >> { \
> >> { \
> >> @@ -123,10 +133,6 @@ static const struct iio_chan_spec mcp9600_channels[][2] = {
> >> MCP9600_CHANNELS(2, 0, 2, 0), /* Alerts: 1 2 3 4 */
> >> };
> >>
> >> -struct mcp9600_data {
> >> - struct i2c_client *client;
> >> -};
> >> -
> >
> > It's not obvious why this piece of change is needed. AFAICS it's a stray change.
>
> The explanation is in the changelog above. A follow up patch needs both struct
> declarations to be where I added one and moved mcp9600_data to. It’s just ordering
> so I don’t later have to forward declare new functions for filter_type, which make
> use of these structs, but need to be in the iio_chan_spec mcp9600_channels[]
> declaration.
>
> I guess I could move mcp9600_data in that series, but I had this in here before
> I split that series out, and it seemed simple enough to leave in.
The usual thing is to avoid changes that are not used. In this series this move
is not used, put it to the patch which actually needs it.
--
With Best Regards,
Andy Shevchenko
> On Aug 20, 2025, at 7:11 AM, Ben Collins <bcollins@watter.com> wrote:
>
>>
>> On Aug 20, 2025, at 6:07 AM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>
>> On Wed, Aug 20, 2025 at 2:45 AM Ben Collins <bcollins@watter.com> wrote:
>>>
>>> The current driver works with mcp9601, but emits a warning because it
>>> does not recognize the chip id.
>>>
>>> MCP9601 is a superset of MCP9600. The drivers works without changes
>>> on this chipset.
>>>
>>> However, the 9601 chip supports open/closed-circuit detection if wired
>>> properly, so we'll need to be able to differentiate between them.
>>>
>>> Moved "struct mcp9600_data" up in the file since a later patch will
>>> need it and chip_info before the declerations.
>>
>> declarations
>>
>> ...
>>
>>> +struct mcp9600_data {
>>> + struct i2c_client *client;
>>> +};
>>> +
>>> #define MCP9600_CHANNELS(hj_num_ev, hj_ev_spec_off, cj_num_ev, cj_ev_spec_off) \
>>> { \
>>> { \
>>> @@ -123,10 +133,6 @@ static const struct iio_chan_spec mcp9600_channels[][2] = {
>>> MCP9600_CHANNELS(2, 0, 2, 0), /* Alerts: 1 2 3 4 */
>>> };
>>>
>>> -struct mcp9600_data {
>>> - struct i2c_client *client;
>>> -};
>>> -
>>
>> It's not obvious why this piece of change is needed. AFAICS it's a stray change.
>
> The explanation is in the changelog above. A follow up patch needs both struct
> declarations to be where I added one and moved mcp9600_data to. It’s just ordering
> so I don’t later have to forward declare new functions for filter_type, which make
> use of these structs, but need to be in the iio_chan_spec mcp9600_channels[]
> declaration.
>
> I guess I could move mcp9600_data in that series, but I had this in here before
> I split that series out, and it seemed simple enough to leave in.
Correction, patch 5/5 needs it. Either I move it in 4/5 (cleaner since I was
already adding a new related struct), or in 5/5, or a longer reorg to include
moving that struct and mcp9600_config() in the mcp9600-iir series.
© 2016 - 2025 Red Hat, Inc.