Re: [PATCH 2/2] iio: temperature: Add driver for NXP P3T175x temperature sensor.

[Andy Shevchenko <andriy.shevchenko@intel.com>]

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