| Message ID | 20221221092801.1977499-4-m.felsch@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Add TI TMP116 Support | expand |
Hi Jonathan, On 22-12-23, Jonathan Cameron wrote: > On Wed, 21 Dec 2022 10:28:00 +0100 > Marco Felsch <m.felsch@pengutronix.de> wrote: > > > The TMP116 is the predecessor of the TMP117. The TMP116 don't support > > custom offset calibration data, instead this register is used as generic > > EEPROM storage as well. > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > A few comments inline. > Thanks, > > Jonathan > > > --- > > v2: > > - no changes > > > > drivers/iio/temperature/tmp117.c | 40 ++++++++++++++++++++++---------- > > 1 file changed, 28 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/iio/temperature/tmp117.c b/drivers/iio/temperature/tmp117.c > > index f9b8f2b570f6..468dafa6fa8e 100644 > > --- a/drivers/iio/temperature/tmp117.c > > +++ b/drivers/iio/temperature/tmp117.c > > @@ -31,9 +31,11 @@ > > #define TMP117_REG_DEVICE_ID 0xF > > > > #define TMP117_RESOLUTION_10UC 78125 > > -#define TMP117_DEVICE_ID 0x0117 > > #define MICRODEGREE_PER_10MILLIDEGREE 10000 > > > > +#define TMP116_DEVICE_ID 0x1116 > > +#define TMP117_DEVICE_ID 0x0117 > > + > > struct tmp117_data { > > struct i2c_client *client; > > s16 calibbias; > > @@ -105,6 +107,13 @@ static const struct iio_chan_spec tmp117_channels[] = { > > .type = IIO_TEMP, > > .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > > BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_SCALE), > > +}; > > + > > +static const struct iio_chan_spec tmp116_channels[] = { > > + { > > + .type = IIO_TEMP, > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > > + BIT(IIO_CHAN_INFO_SCALE), > > }, > > }; > > > > @@ -118,27 +127,28 @@ static int tmp117_identify(struct i2c_client *client) > > int dev_id; > > > > dev_id = i2c_smbus_read_word_swapped(client, TMP117_REG_DEVICE_ID); > > - if (dev_id < 0) > > Keep this handling of the smbus read returning an error. > Otherwise, you end up replacing the error code with -ENODEV rather than > returning what actually happened. > > if (dev_id < 0) > return dev_id; You're right, I will change this thanks. > switch (dev_id) { > ... > > > + switch (dev_id) { > > + case TMP116_DEVICE_ID: > > + case TMP117_DEVICE_ID: > > return dev_id; > > - if (dev_id != TMP117_DEVICE_ID) { > > - dev_err(&client->dev, "TMP117 not found\n"); > > + default: > > + dev_err(&client->dev, "TMP116/117 not found\n"); > > return -ENODEV; > > } > > - return 0; > > } > > > > static int tmp117_probe(struct i2c_client *client) > > { > > struct tmp117_data *data; > > struct iio_dev *indio_dev; > > - int ret; > > + int dev_id; > > > > if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA)) > > return -EOPNOTSUPP; > > > > - ret = tmp117_identify(client); > > - if (ret < 0) > > - return ret; > > + dev_id = tmp117_identify(client); > > + if (dev_id < 0) > > + return dev_id; > > > > indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); > > if (!indio_dev) > > @@ -148,12 +158,18 @@ static int tmp117_probe(struct i2c_client *client) > > data->client = client; > > data->calibbias = 0; > > > > - indio_dev->name = "tmp117"; > > indio_dev->modes = INDIO_DIRECT_MODE; > > indio_dev->info = &tmp117_info; > > > > - indio_dev->channels = tmp117_channels; > > - indio_dev->num_channels = ARRAY_SIZE(tmp117_channels); > > + if (dev_id == TMP117_DEVICE_ID) { > > Probably better to assume we may get more parts in future and use a > switch statement here to explicitly match each value. As you want, I will change it. Regards, Marco > > + indio_dev->channels = tmp117_channels; > > + indio_dev->num_channels = ARRAY_SIZE(tmp117_channels); > > + indio_dev->name = "tmp117"; > > + } else { > > + indio_dev->channels = tmp116_channels; > > + indio_dev->num_channels = ARRAY_SIZE(tmp116_channels); > > + indio_dev->name = "tmp116"; > > + } > > > > return devm_iio_device_register(&client->dev, indio_dev); > > } > >
On Wed, 21 Dec 2022 10:28:00 +0100 Marco Felsch <m.felsch@pengutronix.de> wrote: > The TMP116 is the predecessor of the TMP117. The TMP116 don't support > custom offset calibration data, instead this register is used as generic > EEPROM storage as well. > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> A few comments inline. Thanks, Jonathan > --- > v2: > - no changes > > drivers/iio/temperature/tmp117.c | 40 ++++++++++++++++++++++---------- > 1 file changed, 28 insertions(+), 12 deletions(-) > > diff --git a/drivers/iio/temperature/tmp117.c b/drivers/iio/temperature/tmp117.c > index f9b8f2b570f6..468dafa6fa8e 100644 > --- a/drivers/iio/temperature/tmp117.c > +++ b/drivers/iio/temperature/tmp117.c > @@ -31,9 +31,11 @@ > #define TMP117_REG_DEVICE_ID 0xF > > #define TMP117_RESOLUTION_10UC 78125 > -#define TMP117_DEVICE_ID 0x0117 > #define MICRODEGREE_PER_10MILLIDEGREE 10000 > > +#define TMP116_DEVICE_ID 0x1116 > +#define TMP117_DEVICE_ID 0x0117 > + > struct tmp117_data { > struct i2c_client *client; > s16 calibbias; > @@ -105,6 +107,13 @@ static const struct iio_chan_spec tmp117_channels[] = { > .type = IIO_TEMP, > .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_SCALE), > +}; > + > +static const struct iio_chan_spec tmp116_channels[] = { > + { > + .type = IIO_TEMP, > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > + BIT(IIO_CHAN_INFO_SCALE), > }, > }; > > @@ -118,27 +127,28 @@ static int tmp117_identify(struct i2c_client *client) > int dev_id; > > dev_id = i2c_smbus_read_word_swapped(client, TMP117_REG_DEVICE_ID); > - if (dev_id < 0) Keep this handling of the smbus read returning an error. Otherwise, you end up replacing the error code with -ENODEV rather than returning what actually happened. if (dev_id < 0) return dev_id; switch (dev_id) { ... > + switch (dev_id) { > + case TMP116_DEVICE_ID: > + case TMP117_DEVICE_ID: > return dev_id; > - if (dev_id != TMP117_DEVICE_ID) { > - dev_err(&client->dev, "TMP117 not found\n"); > + default: > + dev_err(&client->dev, "TMP116/117 not found\n"); > return -ENODEV; > } > - return 0; > } > > static int tmp117_probe(struct i2c_client *client) > { > struct tmp117_data *data; > struct iio_dev *indio_dev; > - int ret; > + int dev_id; > > if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA)) > return -EOPNOTSUPP; > > - ret = tmp117_identify(client); > - if (ret < 0) > - return ret; > + dev_id = tmp117_identify(client); > + if (dev_id < 0) > + return dev_id; > > indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); > if (!indio_dev) > @@ -148,12 +158,18 @@ static int tmp117_probe(struct i2c_client *client) > data->client = client; > data->calibbias = 0; > > - indio_dev->name = "tmp117"; > indio_dev->modes = INDIO_DIRECT_MODE; > indio_dev->info = &tmp117_info; > > - indio_dev->channels = tmp117_channels; > - indio_dev->num_channels = ARRAY_SIZE(tmp117_channels); > + if (dev_id == TMP117_DEVICE_ID) { Probably better to assume we may get more parts in future and use a switch statement here to explicitly match each value. > + indio_dev->channels = tmp117_channels; > + indio_dev->num_channels = ARRAY_SIZE(tmp117_channels); > + indio_dev->name = "tmp117"; > + } else { > + indio_dev->channels = tmp116_channels; > + indio_dev->num_channels = ARRAY_SIZE(tmp116_channels); > + indio_dev->name = "tmp116"; > + } > > return devm_iio_device_register(&client->dev, indio_dev); > }
On Fri, 23 Dec 2022 16:07:28 +0100 Marco Felsch <m.felsch@pengutronix.de> wrote: > Hi Jonathan, > > On 22-12-23, Jonathan Cameron wrote: > > On Wed, 21 Dec 2022 10:28:00 +0100 > > Marco Felsch <m.felsch@pengutronix.de> wrote: > > > > > The TMP116 is the predecessor of the TMP117. The TMP116 don't support > > > custom offset calibration data, instead this register is used as generic > > > EEPROM storage as well. > > > > > > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> > > A few comments inline. > > Thanks, > > > > Jonathan > > > > > --- > > > v2: > > > - no changes > > > > > > drivers/iio/temperature/tmp117.c | 40 ++++++++++++++++++++++---------- > > > 1 file changed, 28 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/iio/temperature/tmp117.c b/drivers/iio/temperature/tmp117.c > > > index f9b8f2b570f6..468dafa6fa8e 100644 > > > --- a/drivers/iio/temperature/tmp117.c > > > +++ b/drivers/iio/temperature/tmp117.c > > > @@ -31,9 +31,11 @@ > > > #define TMP117_REG_DEVICE_ID 0xF > > > > > > #define TMP117_RESOLUTION_10UC 78125 > > > -#define TMP117_DEVICE_ID 0x0117 > > > #define MICRODEGREE_PER_10MILLIDEGREE 10000 > > > > > > +#define TMP116_DEVICE_ID 0x1116 > > > +#define TMP117_DEVICE_ID 0x0117 > > > + > > > struct tmp117_data { > > > struct i2c_client *client; > > > s16 calibbias; > > > @@ -105,6 +107,13 @@ static const struct iio_chan_spec tmp117_channels[] = { > > > .type = IIO_TEMP, > > > .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > > > BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_SCALE), > > > +}; > > > + > > > +static const struct iio_chan_spec tmp116_channels[] = { > > > + { > > > + .type = IIO_TEMP, > > > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | > > > + BIT(IIO_CHAN_INFO_SCALE), > > > }, > > > }; > > > > > > @@ -118,27 +127,28 @@ static int tmp117_identify(struct i2c_client *client) > > > int dev_id; > > > > > > dev_id = i2c_smbus_read_word_swapped(client, TMP117_REG_DEVICE_ID); > > > - if (dev_id < 0) > > > > Keep this handling of the smbus read returning an error. > > Otherwise, you end up replacing the error code with -ENODEV rather than > > returning what actually happened. > > > > if (dev_id < 0) > > return dev_id; > > You're right, I will change this thanks. > > > switch (dev_id) { > > ... > > > > > + switch (dev_id) { > > > + case TMP116_DEVICE_ID: > > > + case TMP117_DEVICE_ID: > > > return dev_id; > > > - if (dev_id != TMP117_DEVICE_ID) { > > > - dev_err(&client->dev, "TMP117 not found\n"); > > > + default: > > > + dev_err(&client->dev, "TMP116/117 not found\n"); > > > return -ENODEV; As per the other branch of this thread. This isn't an error. If we want fallback compatibles to work in their role of allowing for newer devices that are actually compatible, the most we should do here is warn. Say a new tmp117b device is released. It's fully backwards compatible with the exception of an ID - or supports only new features + backwards compatibility then that would have a fallback to tmp117 and we need it to work. > > > } > > > - return 0; > > > } > > > > > > static int tmp117_probe(struct i2c_client *client) > > > { > > > struct tmp117_data *data; > > > struct iio_dev *indio_dev; > > > - int ret; > > > + int dev_id; > > > > > > if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA)) > > > return -EOPNOTSUPP; > > > > > > - ret = tmp117_identify(client); > > > - if (ret < 0) > > > - return ret; > > > + dev_id = tmp117_identify(client); > > > + if (dev_id < 0) > > > + return dev_id; > > > > > > indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); > > > if (!indio_dev) > > > @@ -148,12 +158,18 @@ static int tmp117_probe(struct i2c_client *client) > > > data->client = client; > > > data->calibbias = 0; > > > > > > - indio_dev->name = "tmp117"; > > > indio_dev->modes = INDIO_DIRECT_MODE; > > > indio_dev->info = &tmp117_info; > > > > > > - indio_dev->channels = tmp117_channels; > > > - indio_dev->num_channels = ARRAY_SIZE(tmp117_channels); > > > + if (dev_id == TMP117_DEVICE_ID) { > > > > Probably better to assume we may get more parts in future and use a > > switch statement here to explicitly match each value. > > As you want, I will change it. > > Regards, > Marco > > > > + indio_dev->channels = tmp117_channels; > > > + indio_dev->num_channels = ARRAY_SIZE(tmp117_channels); > > > + indio_dev->name = "tmp117"; > > > + } else { > > > + indio_dev->channels = tmp116_channels; > > > + indio_dev->num_channels = ARRAY_SIZE(tmp116_channels); > > > + indio_dev->name = "tmp116"; > > > + } > > > > > > return devm_iio_device_register(&client->dev, indio_dev); > > > } > > > >
On 22-12-23, Jonathan Cameron wrote: ... > > > > @@ -118,27 +127,28 @@ static int tmp117_identify(struct i2c_client *client) > > > > int dev_id; > > > > > > > > dev_id = i2c_smbus_read_word_swapped(client, TMP117_REG_DEVICE_ID); > > > > - if (dev_id < 0) > > > > > > Keep this handling of the smbus read returning an error. > > > Otherwise, you end up replacing the error code with -ENODEV rather than > > > returning what actually happened. > > > > > > if (dev_id < 0) > > > return dev_id; > > > > You're right, I will change this thanks. > > > > > switch (dev_id) { > > > ... > > > > > > > + switch (dev_id) { > > > > + case TMP116_DEVICE_ID: > > > > + case TMP117_DEVICE_ID: > > > > return dev_id; > > > > - if (dev_id != TMP117_DEVICE_ID) { > > > > - dev_err(&client->dev, "TMP117 not found\n"); > > > > + default: > > > > + dev_err(&client->dev, "TMP116/117 not found\n"); > > > > return -ENODEV; > > As per the other branch of this thread. This isn't an error. > If we want fallback compatibles to work in their role of allowing > for newer devices that are actually compatible, the most we should > do here is warn. > > Say a new tmp117b device is released. It's fully backwards compatible > with the exception of an ID - or supports only new features + backwards > compatibility then that would have a fallback to tmp117 and we need > it to work. This isn't part of this patchset and IMHO implementing something which may happen in the future is not the way we should go. Regards, Marco
On Fri, 23 Dec 2022 17:13:59 +0100 Marco Felsch <m.felsch@pengutronix.de> wrote: > On 22-12-23, Jonathan Cameron wrote: > > ... > > > > > > @@ -118,27 +127,28 @@ static int tmp117_identify(struct i2c_client *client) > > > > > int dev_id; > > > > > > > > > > dev_id = i2c_smbus_read_word_swapped(client, TMP117_REG_DEVICE_ID); > > > > > - if (dev_id < 0) > > > > > > > > Keep this handling of the smbus read returning an error. > > > > Otherwise, you end up replacing the error code with -ENODEV rather than > > > > returning what actually happened. > > > > > > > > if (dev_id < 0) > > > > return dev_id; > > > > > > You're right, I will change this thanks. > > > > > > > switch (dev_id) { > > > > ... > > > > > > > > > + switch (dev_id) { > > > > > + case TMP116_DEVICE_ID: > > > > > + case TMP117_DEVICE_ID: > > > > > return dev_id; > > > > > - if (dev_id != TMP117_DEVICE_ID) { > > > > > - dev_err(&client->dev, "TMP117 not found\n"); > > > > > + default: > > > > > + dev_err(&client->dev, "TMP116/117 not found\n"); > > > > > return -ENODEV; > > > > As per the other branch of this thread. This isn't an error. > > If we want fallback compatibles to work in their role of allowing > > for newer devices that are actually compatible, the most we should > > do here is warn. > > > > Say a new tmp117b device is released. It's fully backwards compatible > > with the exception of an ID - or supports only new features + backwards > > compatibility then that would have a fallback to tmp117 and we need > > it to work. > > This isn't part of this patchset and IMHO implementing something which > may happen in the future is not the way we should go. I held a similar view, but the response I got from the DT maintainers was that a driver should not reject a DTS that says it is compatible based on an unknown ID - because it prevents that case of an old kernel working absolutely fine with a completely compatible newer part. Jonathan > > Regards, > Marco
On 23/12/2022 18:16, Jonathan Cameron wrote: > On Fri, 23 Dec 2022 17:13:59 +0100 > Marco Felsch <m.felsch@pengutronix.de> wrote: > >> On 22-12-23, Jonathan Cameron wrote: >> >> ... >> >>>>>> @@ -118,27 +127,28 @@ static int tmp117_identify(struct i2c_client *client) >>>>>> int dev_id; >>>>>> >>>>>> dev_id = i2c_smbus_read_word_swapped(client, TMP117_REG_DEVICE_ID); >>>>>> - if (dev_id < 0) >>>>> >>>>> Keep this handling of the smbus read returning an error. >>>>> Otherwise, you end up replacing the error code with -ENODEV rather than >>>>> returning what actually happened. >>>>> >>>>> if (dev_id < 0) >>>>> return dev_id; >>>> >>>> You're right, I will change this thanks. >>>> >>>>> switch (dev_id) { >>>>> ... >>>>> >>>>>> + switch (dev_id) { >>>>>> + case TMP116_DEVICE_ID: >>>>>> + case TMP117_DEVICE_ID: >>>>>> return dev_id; >>>>>> - if (dev_id != TMP117_DEVICE_ID) { >>>>>> - dev_err(&client->dev, "TMP117 not found\n"); >>>>>> + default: >>>>>> + dev_err(&client->dev, "TMP116/117 not found\n"); >>>>>> return -ENODEV; >>> >>> As per the other branch of this thread. This isn't an error. >>> If we want fallback compatibles to work in their role of allowing >>> for newer devices that are actually compatible, the most we should >>> do here is warn. >>> >>> Say a new tmp117b device is released. It's fully backwards compatible >>> with the exception of an ID - or supports only new features + backwards >>> compatibility then that would have a fallback to tmp117 and we need >>> it to work. >> >> This isn't part of this patchset and IMHO implementing something which >> may happen in the future is not the way we should go. > > I held a similar view, but the response I got from the DT maintainers was > that a driver should not reject a DTS that says it is compatible based > on an unknown ID - because it prevents that case of an old kernel working > absolutely fine with a completely compatible newer part. I don't think that there was such generic recommendation. Accepting unknown devices (unknown register IDs) is a risk - device might behave correct or not. If it is a critical device, like regulator, misbehave might damage something. What's more, how Linux driver behaves on device IDs (not compatibles) is also a bit outside of DT scope. If a driver claims it handles compatibles tmp117, then indeed it should work fine with any DTS node claiming to be compatible with tmp117. However driver is free to make further checks (if possible) whether the device (e.g. tmp116 or tmp11X) is really compatible and reject unknown devices for safety reason. The same as x86 kernel is fine to reject to work on newest (unknown) x86 processors for safety reasons... which is terrible from user-experience point of view unless it is real safety case. Best regards, Krzysztof
On Tue, 27 Dec 2022 09:30:08 +0100 Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote: > On 23/12/2022 18:16, Jonathan Cameron wrote: > > On Fri, 23 Dec 2022 17:13:59 +0100 > > Marco Felsch <m.felsch@pengutronix.de> wrote: > > > >> On 22-12-23, Jonathan Cameron wrote: > >> > >> ... > >> > >>>>>> @@ -118,27 +127,28 @@ static int tmp117_identify(struct i2c_client *client) > >>>>>> int dev_id; > >>>>>> > >>>>>> dev_id = i2c_smbus_read_word_swapped(client, TMP117_REG_DEVICE_ID); > >>>>>> - if (dev_id < 0) > >>>>> > >>>>> Keep this handling of the smbus read returning an error. > >>>>> Otherwise, you end up replacing the error code with -ENODEV rather than > >>>>> returning what actually happened. > >>>>> > >>>>> if (dev_id < 0) > >>>>> return dev_id; > >>>> > >>>> You're right, I will change this thanks. > >>>> > >>>>> switch (dev_id) { > >>>>> ... > >>>>> > >>>>>> + switch (dev_id) { > >>>>>> + case TMP116_DEVICE_ID: > >>>>>> + case TMP117_DEVICE_ID: > >>>>>> return dev_id; > >>>>>> - if (dev_id != TMP117_DEVICE_ID) { > >>>>>> - dev_err(&client->dev, "TMP117 not found\n"); > >>>>>> + default: > >>>>>> + dev_err(&client->dev, "TMP116/117 not found\n"); > >>>>>> return -ENODEV; > >>> > >>> As per the other branch of this thread. This isn't an error. > >>> If we want fallback compatibles to work in their role of allowing > >>> for newer devices that are actually compatible, the most we should > >>> do here is warn. > >>> > >>> Say a new tmp117b device is released. It's fully backwards compatible > >>> with the exception of an ID - or supports only new features + backwards > >>> compatibility then that would have a fallback to tmp117 and we need > >>> it to work. > >> > >> This isn't part of this patchset and IMHO implementing something which > >> may happen in the future is not the way we should go. > > > > I held a similar view, but the response I got from the DT maintainers was > > that a driver should not reject a DTS that says it is compatible based > > on an unknown ID - because it prevents that case of an old kernel working > > absolutely fine with a completely compatible newer part. > > I don't think that there was such generic recommendation. Accepting > unknown devices (unknown register IDs) is a risk - device might behave > correct or not. If it is a critical device, like regulator, misbehave > might damage something. Agreed - I didn't express that there are limits to such a requirement. Indeed not a good idea with regulators etc! However, for input devices like this one things are a little simpler - in theory they could be used for something that ends up damaging hardware if done wrong, but it's much less likely. > > What's more, how Linux driver behaves on device IDs (not compatibles) is > also a bit outside of DT scope. > > If a driver claims it handles compatibles tmp117, then indeed it should > work fine with any DTS node claiming to be compatible with tmp117. > However driver is free to make further checks (if possible) whether the > device (e.g. tmp116 or tmp11X) is really compatible and reject unknown > devices for safety reason. Ok. For input devices at least in IIO we went around this a few times and ended up with deciding that a dev_info() type message was the best balance. We will need to be more careful for output devices. > > The same as x86 kernel is fine to reject to work on newest (unknown) x86 > processors for safety reasons... which is terrible from user-experience > point of view unless it is real safety case. Hopefully that never happens :) Jonathan > > Best regards, > Krzysztof >
diff --git a/drivers/iio/temperature/tmp117.c b/drivers/iio/temperature/tmp117.c index f9b8f2b570f6..468dafa6fa8e 100644 --- a/drivers/iio/temperature/tmp117.c +++ b/drivers/iio/temperature/tmp117.c @@ -31,9 +31,11 @@ #define TMP117_REG_DEVICE_ID 0xF #define TMP117_RESOLUTION_10UC 78125 -#define TMP117_DEVICE_ID 0x0117 #define MICRODEGREE_PER_10MILLIDEGREE 10000 +#define TMP116_DEVICE_ID 0x1116 +#define TMP117_DEVICE_ID 0x0117 + struct tmp117_data { struct i2c_client *client; s16 calibbias; @@ -105,6 +107,13 @@ static const struct iio_chan_spec tmp117_channels[] = { .type = IIO_TEMP, .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_CALIBBIAS) | BIT(IIO_CHAN_INFO_SCALE), +}; + +static const struct iio_chan_spec tmp116_channels[] = { + { + .type = IIO_TEMP, + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | + BIT(IIO_CHAN_INFO_SCALE), }, }; @@ -118,27 +127,28 @@ static int tmp117_identify(struct i2c_client *client) int dev_id; dev_id = i2c_smbus_read_word_swapped(client, TMP117_REG_DEVICE_ID); - if (dev_id < 0) + switch (dev_id) { + case TMP116_DEVICE_ID: + case TMP117_DEVICE_ID: return dev_id; - if (dev_id != TMP117_DEVICE_ID) { - dev_err(&client->dev, "TMP117 not found\n"); + default: + dev_err(&client->dev, "TMP116/117 not found\n"); return -ENODEV; } - return 0; } static int tmp117_probe(struct i2c_client *client) { struct tmp117_data *data; struct iio_dev *indio_dev; - int ret; + int dev_id; if (!i2c_check_functionality(client->adapter, I2C_FUNC_SMBUS_WORD_DATA)) return -EOPNOTSUPP; - ret = tmp117_identify(client); - if (ret < 0) - return ret; + dev_id = tmp117_identify(client); + if (dev_id < 0) + return dev_id; indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); if (!indio_dev) @@ -148,12 +158,18 @@ static int tmp117_probe(struct i2c_client *client) data->client = client; data->calibbias = 0; - indio_dev->name = "tmp117"; indio_dev->modes = INDIO_DIRECT_MODE; indio_dev->info = &tmp117_info; - indio_dev->channels = tmp117_channels; - indio_dev->num_channels = ARRAY_SIZE(tmp117_channels); + if (dev_id == TMP117_DEVICE_ID) { + indio_dev->channels = tmp117_channels; + indio_dev->num_channels = ARRAY_SIZE(tmp117_channels); + indio_dev->name = "tmp117"; + } else { + indio_dev->channels = tmp116_channels; + indio_dev->num_channels = ARRAY_SIZE(tmp116_channels); + indio_dev->name = "tmp116"; + } return devm_iio_device_register(&client->dev, indio_dev); }
The TMP116 is the predecessor of the TMP117. The TMP116 don't support custom offset calibration data, instead this register is used as generic EEPROM storage as well. Signed-off-by: Marco Felsch <m.felsch@pengutronix.de> --- v2: - no changes drivers/iio/temperature/tmp117.c | 40 ++++++++++++++++++++++---------- 1 file changed, 28 insertions(+), 12 deletions(-)