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

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <aIIgHV38kKsPVCUN@smile.fi.intel.com>
Date: Thu, 24 Jul 2025 14:59:25 +0300
From: Andy Shevchenko <andriy.shevchenko@...el.com>
To: Lakshay Piplani <lakshay.piplani@....com>
Cc: linux-kernel@...r.kernel.org, linux-iio@...r.kernel.org,
	jic23@...nel.org, dlechner@...libre.com, nuno.sa@...log.com,
	andy@...nel.org, marcelo.schmitt1@...il.com,
	gregkh@...uxfoundation.org, viro@...iv.linux.org.uk,
	peterz@...radead.org, jstephan@...libre.com, robh@...nel.org,
	krzk+dt@...nel.org, conor+dt@...nel.org, devicetree@...r.kernel.org,
	vikash.bansal@....com, priyanka.jain@....com,
	shashank.rebbapragada@....com, Frank.Li@....com,
	carlos.song@....com, xiaoning.wang@....com, haibo.chen@....com
Subject: Re: [PATCH 2/2] iio: temperature: Add driver for NXP P3T175x
 temperature sensor.

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