| 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: <CAHp75Vd-3OwHfyDSH_+GtbD8zNaz8qi2WQZD1kHM0GfUnRXRnA@mail.gmail.com>
Date: Wed, 13 Aug 2025 12:32:34 +0200
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Yusuf Alper Bilgin <y.alperbilgin@...il.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Michael Hennerich <Michael.Hennerich@...log.com>,
Jonathan Cameron <jic23@...nel.org>, David Lechner <dlechner@...libre.com>,
Nuno Sá <nuno.sa@...log.com>,
Andy Shevchenko <andy@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Liam Beguin <liambeguin@...il.com>, linux-iio@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/3] iio: adc: ltc2497: add temperature sensor support
On Tue, Aug 12, 2025 at 7:09 PM Yusuf Alper Bilgin
<y.alperbilgin@...il.com> wrote:
>
> The LTC2495 and LTC2499 include an internal temperature sensor. This
> patch adds support for reading it via a standard IIO temperature
> channel.
..
> static const struct ltc2497_chip_info ltc2496_info = {
> .resolution = 16,
> .name = NULL,
> + .has_temp_channel = false,
> };
Unneeded change.
...
> static int ltc2497core_read_raw(struct iio_dev *indio_dev,
> - struct iio_chan_spec const *chan,
> - int *val, int *val2, long mask)
> + struct iio_chan_spec const *chan,
> + int *val, int *val2, long mask)
Unrelated change.
...
> + switch (chan->type) {
> + case IIO_VOLTAGE:
> + *val = ret / 1000;
Don't remember if we have something like MICROVOLT_PER_MILLIVOLT.
> + *val2 = ddata->chip_info->resolution + 1;
> +
> + return IIO_VAL_FRACTIONAL_LOG2;
> +
> + case IIO_TEMP:
> + if (!ddata->chip_info->has_temp_channel)
> + return -EINVAL;
> +
> + /*
> + * The datasheet formula to get Temperature in Celsius is:
> + * Temp_C = (Conversion * Vref / temp_scale) - 273
> + *
> + * To match the IIO framework's model of (raw + offset) * scale,
> + * and to get the final result in millidegrees Celsius:
> + *
> + * = ((Conversion * Vref / temp_scale) - 273) * 1000
> + * = (Conversion - (273 * temp_scale / Vref)) * 1000 * Vref / temp_scale
* Temp_mC = ... =
* ...
I.o.w. make it more explicit, otherwise those dangling equal signs are
not helpful.
> + *
> + * This gives us if the Vref is in mV:
> + * scale = Vref * 1000 / temp_scale
> + * offset = -273 * temp_scale / Vref
> + */
> + *val = ret;
> + *val2 = ddata->chip_info->temp_scale;
> +
> + return IIO_VAL_FRACTIONAL;
> +
> + default:
> + return -EINVAL;
> + }
> + case IIO_CHAN_INFO_OFFSET:
> + if (chan->type != IIO_TEMP)
> + return -EINVAL;
> +
> + /* see the calculation above. Offset with (-273 * temp_scale / Vref) */
> + ret = regulator_get_voltage(ddata->ref);
> + if (ret < 0)
> + return ret;
>
> - return IIO_VAL_FRACTIONAL_LOG2;
> + *val = -273 * ddata->chip_info->temp_scale;
> + *val2 = ret / 1000;
I remember we have some constants in units.h for degrees, but don't
remember if they are for Kelvin only or Celsius also included. Please,
check and use, if available.
> + return IIO_VAL_FRACTIONAL;
>
> default:
> return -EINVAL;
> }
...
> +#define LTC2497_T_CHAN() { \
Why parentheses?
> + .type = IIO_TEMP, \
> + .channel = 0, \
> + .address = (LTC2497_TEMP_CMD_ADDR), \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW), \
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SCALE) | BIT(IIO_CHAN_INFO_OFFSET), \
> +}
...
> + /* Dynamically create the channel list */
> + if (ddata->chip_info->has_temp_channel) {
> + /* Allocate space for common channels + 1 temp channel */
> + indio_dev->num_channels = ARRAY_SIZE(ltc2497core_channel) + 1;
> + indio_dev->channels = devm_kmemdup(dev, ltc2497core_channel,
Why not devm_kmemdup_array() ?
> + sizeof(ltc2497core_channel), GFP_KERNEL);
sizeof(*...)
> + if (!indio_dev->channels)
> + return -ENOMEM;
> +
> + memcpy((void *)&indio_dev->channels[ARRAY_SIZE(ltc2497core_channel)],
This is a strange style, can you improve it?
> + <c2497_temp_channel, sizeof(ltc2497_temp_channel));
> + } else {
> + indio_dev->channels = ltc2497core_channel;
> + indio_dev->num_channels = ARRAY_SIZE(ltc2497core_channel);
> + }
...
> + if (ddata->chip_info->has_temp_channel) {
> + if (address == LTC2497_TEMP_CMD_ADDR)
> + ret = i2c_smbus_write_byte_data(st->client, LTC2497_ENABLE,
> + LTC2497_TEMP_CMD_ADDR);
> + else
> + ret = i2c_smbus_write_byte_data(st->client, LTC2497_ENABLE | address,
> + LTC2497_EN2);
> +
Stray blank line.
> + } else {
> + ret = i2c_smbus_write_byte(st->client, LTC2497_ENABLE | address);
> + }
> +
> if (ret)
> - dev_err(&st->client->dev, "i2c transfer failed: %pe\n",
> - ERR_PTR(ret));
> + dev_err(&st->client->dev, "i2c transfer failed: %pe\n", ERR_PTR(ret));
> return ret;
...
> [TYPE_LTC2497] = {
> .resolution = 16,
> .name = NULL,
> + .has_temp_channel = false,
> },
Unneeded change.
...
> [TYPE_LTC2499] = {
> .resolution = 24,
> .name = "ltc2499",
> + .has_temp_channel = true,
> + .temp_scale = 1570000, /* 1570 V per degree C -> 1570000 mV */
1570V ?! Hmm...
> },
...
> +#define LTC2497_ENABLE 0xA0
> +#define LTC2497_EN2 0x80
> +#define LTC2497_IM 0x40
Are those bit masks / bits in the register? Offsets?
> +#define LTC2497_TEMP_CMD_ADDR (LTC2497_EN2 | LTC2497_IM)
This is really strange naming for... what exactly? Can a comment be added?
...
> struct ltc2497_chip_info {
> u32 resolution;
> const char *name;
> + bool has_temp_channel;
> + u32 temp_scale; /* in mV, for temp conversion */
Then simply add a (mV, yes, with capital V) suffix to the field name.
> };
Have you run `pahole`? Does it agree with the proposed layout?
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists