On Wed, 21 Dec 2022 10:28:00 +0100
Marco Felsch <m.felsch@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx>
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);
> }
