Re: [PATCH 2/2] iio: temperature: Add driver for NXP P3T175x temperature sensor.
From: Andy Shevchenko
Date: Thu Jul 24 2025 - 07:59:49 EST
On Thu, Jul 24, 2025 at 02:09:51PM +0530, Lakshay Piplani wrote:
> Add support for the NXP P3T175x (P3T1755/P3T1750)
> family of temperature sensor devices. These devices
> communicates via both I2C or I3C interfaces.
...
> drivers/iio/temperature/p3t/Kconfig | 89 ++++
> drivers/iio/temperature/p3t/Makefile | 5 +
> drivers/iio/temperature/p3t/p3t1755.h | 60 +++
> drivers/iio/temperature/p3t/p3t1755_core.c | 513 +++++++++++++++++++++
> drivers/iio/temperature/p3t/p3t1755_i2c.c | 142 ++++++
> drivers/iio/temperature/p3t/p3t1755_i3c.c | 147 ++++++
Please, split glue drivers in a separate patches. At bare minimum it will help
reviewing the core part.
...
> +// Conversion rate table: maps bits to sampling frequency
> +static const struct {
> + u8 bits;
> + int freq_hz;
Can frequency be negative?
> +} p3t1755_samp_freqs[] = {
> + { 0x00, 36 }, // 27.5 ms
> + { 0x01, 18 }, // 55 ms (default)
> + { 0x02, 9 }, // 110 ms
> + { 0x03, 4 }, // 220 ms
If you need ms, make the field to be ms and not Hz.
Otherwise drop unneeded comments. Conversion from Hz to s is straightforward
for the 101 in school for physics.
> +};
...
> +int p3t1755_fault_queue_to_bits(int val)
> +{
> + int i;
Why signed?
> + for (i = 0; i < ARRAY_SIZE(p3t1755_fault_queue_values); i++)
> + if (p3t1755_fault_queue_values[i] == val)
> + return i;
> + return -EINVAL;
> +}
...
> +int p3t1755_get_temp_and_limits(struct p3t1755_data *data,
> + int *temp_mc, int *thigh_mc, int *tlow_mc)
> +{
> + u8 buf[2];
Not a proper bitwise endianess-aware type?
> + int ret;
> +
> + ret = regmap_bulk_read(data->regmap, P3T1755_REG_TEMP, buf, 2);
> + if (ret) {
> + dev_dbg(data->dev, "Failed to read TEMP register: %d\n", ret);
> + return ret;
> + }
> + *temp_mc = (((buf[0] << 8) | buf[1]) >> 4) * P3T1755_RESOLUTION_10UC / 1000;
Use constant from units.h or from time.h.
> + dev_dbg(data->dev, "TEMP raw: 0x%02x%02x, temp_mc: %d\n",
> + buf[0], buf[1], *temp_mc);
Printing raw with proper 126-bit type will be easier.
> + ret = regmap_bulk_read(data->regmap, P3T1755_REG_HIGH_LIM, buf, 2);
sizeof()
> + if (ret) {
> + dev_dbg(data->dev, "Failed to read HIGH_LIM register: %d\n", ret);
> + return ret;
> + }
> + *thigh_mc = (((buf[0] << 8) | buf[1]) >> 4) * P3T1755_RESOLUTION_10UC / 1000;
> + dev_dbg(data->dev, "HIGH_LIM raw: 0x%02x%02x, thigh_mc: %d\n",
> + buf[0], buf[1], *thigh_mc);
> +
> + ret = regmap_bulk_read(data->regmap, P3T1755_REG_LOW_LIM, buf, 2);
> + if (ret) {
> + dev_dbg(data->dev, "Failed to read LOW_LIM register: %d\n", ret);
> + return ret;
> + }
> + *tlow_mc = (((buf[0] << 8) | buf[1]) >> 4) * P3T1755_RESOLUTION_10UC / 1000;
> + dev_dbg(data->dev, "LOW_LIM raw: 0x%02x%02x, tlow_mc: %d\n",
> + buf[0], buf[1], *tlow_mc);
> +
> + dev_dbg(data->dev, "Successfully read all temperature values\n");
> + return 0;
> +}
...
> +#include <linux/kernel.h>
No way this header should be in a new code.
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/slab.h>
> +#include <linux/regmap.h>
> +#include <linux/of.h>
Neither this.
Hint: use device property API (or fwnode in the cases where no struct device is
available).
> +#include <linux/iio/iio.h>
> +#include <linux/iio/events.h>
--
With Best Regards,
Andy Shevchenko