| 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: <CAHp75VdmJMM22DY=WGZpZ82uvXuER_dEnQL8XPx4fFCXvh17TQ@mail.gmail.com>
Date: Fri, 29 Aug 2025 18:43:12 +0300
From: Andy Shevchenko <andy.shevchenko@...il.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, ilpo.jarvinen@...ux.intel.com,
jonathan.cameron@...wei.com, akpm@...ux-foundation.org, chao@...nel.org,
jaegeuk@...nel.org, vikash.bansal@....com, priyanka.jain@....com,
shashank.rebbapragada@....com, Frank.Li@....com
Subject: Re: [PATCH v2 2/2] iio: temperature: Add driver for NXP P3T175x
temperature sensor
On Wed, Aug 27, 2025 at 1:31 PM Lakshay Piplani <lakshay.piplani@....com> wrote:
>
> Add support for the NXP P3T175x (P3T1750/P3T1755) family of temperature
> sensor devices. These devices communicates via both I2C or I3C interfaces.
>
> This driver belongs under IIO because:
> The P3T1750/P3T1755 sensors require interrupt or IBI support to handle
> threshold events, which the hwmon subsystem does not provide. In contrast,
> the IIO subsystem offers robust event handling that matches the hardware
> capabilities of these sensors. Therefore, this driver is better suited
> under IIO.
...
> +Date: August 2025
> +KernelVersion: 6.17
> +Contact: Lakshay Piplani <lakshay.piplani@....com>
...
> This driver can also be built as a module. If so, the module
> will be called mcp9600.
Missed blank line here.
> +source "drivers/iio/temperature/p3t/Kconfig"
>
> endmenu
...
> +config IIO_P3T1755_I2C
> + tristate "NXP P3T1755 temprature sensor I2C driver"
temperature
> + select IIO_P3T1755
> + select REGMAP_I2C
> + help
> + Say yes here to build support for NXP P3T1755 I2C temperature
> + sensor.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called p3t1755_i2c
> +
> +config IIO_P3T1755_I3C
> + tristate "NXP P3T1755 temprature sensor I3C driver"
Ditto.
> + select IIO_P3T1755
> + select REGMAP_I3C
> + depends on I3C
> + help
> + Say yes here to build support for NXP P3T1755 I3C temperature
> + sensor.
> +
> + To compile this driver as a module, choose M here: the module
> + will be called p3t1755_i3c
...
> +#ifndef P3T1755_H
> +#define P3T1755_H
> +
> +#include <linux/device.h>
> +#include <linux/iio/iio.h>
This is definitely an incorrect list of the inclusions. Follow the
IWYU principle (include what you use).
...
> +#define P3T1755_SHUTDOWN_BIT BIT(0)
> +#define P3T1755_TM_BIT BIT(1)
> +#define P3T1755_POL_BIT BIT(2)
> +#define P3T1755_ONE_SHOT_BIT BIT(7)
+bits.h
...
> +extern const struct p3t1755_info p3t1755_channels_info;
> +extern const struct p3t1755_info p3t1750_channels_info;
Please, move this after the actual structure type definition.
...
> +enum p3t1755_hw_id {
> + P3T1755_ID = 0,
> + P3T1750_ID,
> +};
Is this related to HW (like values that are read from HW? If so, make
them all to be explicitly assigned. Otherwise, drop the assignment and
sort the list.
...
> +struct p3t1755_data {
> + struct device *dev;
> + struct regmap *regmap;
> + struct mutex lock; /* Protects access to sensor registers */
+ mutex.h
> + bool tm_mode;
+ types.h
> +};
> +#endif /* P3T1755_H */
The rest can be declared with forward declarations.
...
> +#include <linux/err.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/module.h>
> +#include <linux/bitops.h>
> +#include <linux/bitfield.h>
> +#include <linux/interrupt.h>
> +#include <linux/types.h>
> +#include <linux/regmap.h>
> +#include <linux/device.h>
> +#include <linux/iio/events.h>
Please, keep it ordered, also move iio/* to a separate group
linux/*
...blank line...
linux/iio/*
...
> +static const struct {
> + u8 bits;
> + unsigned int freq_hz;
> +} p3t1755_samp_freqs[] = {
> + { 0x00, 36 },
> + { 0x01, 18 },
> + { 0x02, 9 },
> + { 0x03, 4 },
Is it 4 for real? To me it sounds like it should be 4.5. Also, the
bits field is redundant. Index is the same.
> +};
...
> +int p3t1755_fault_queue_to_bits(int val)
> +{
> + int i;
Here and elsewhere why is 'i' signed?
> + for (i = 0; i < ARRAY_SIZE(p3t1755_fault_queue_values); i++)
> + if (p3t1755_fault_queue_values[i] == val)
> + return i;
> + return -EINVAL;
> +}
...
> +static int p3t1755_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *channel, int *val,
> + int *val2, long mask)
> +{
> + struct p3t1755_data *data = iio_priv(indio_dev);
> + unsigned int cfgr;
> + __be16 be;
> + int ret;
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + ret = regmap_bulk_read(data->regmap, P3T1755_REG_TEMP, &be, sizeof(be));
> + if (ret < 0) {
Here and elsewhere out of a sudden the ' < 0' part. Please, remove it
where it's not needed.
> + dev_err(data->dev, "Failed to read temperature register\n");
> + return ret;
> + }
> + *val = sign_extend32(be16_to_cpu(be) >> 4, 11);
> +
> + return IIO_VAL_INT;
> +
> + case IIO_CHAN_INFO_SCALE:
> + *val = 625;
> + *val2 = 10000;
> +
> + return IIO_VAL_FRACTIONAL;
> +
> + case IIO_CHAN_INFO_ENABLE:
> + ret = regmap_read(data->regmap, P3T1755_REG_CFGR, &cfgr);
> + if (ret < 0) {
> + dev_err(data->dev, "Failed to read configuration register\n");
> + return ret;
> + }
> + *val = !FIELD_GET(P3T1755_SHUTDOWN_BIT, cfgr);
> +
> + return IIO_VAL_INT;
> +
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + u8 sel;
Here and elsewhere, we usually don't allow mix definitions with the
code, only in exceptional cases (RAII, loop iterators). To fix this,
the whole case block should go in curly braces {}.
> + ret = regmap_read(data->regmap, P3T1755_REG_CFGR, &cfgr);
> + if (ret < 0) {
> + dev_err(data->dev, "Failed to read configuration register\n");
> + return ret;
> + }
> +
> + sel = FIELD_GET(P3T1755_CONVERSION_TIME_BITS, cfgr);
> + if (sel >= ARRAY_SIZE(p3t1755_samp_freqs))
> + return -EINVAL;
> +
> + *val = p3t1755_samp_freqs[sel].freq_hz;
> +
> + return IIO_VAL_INT;
> + default:
> + return -EINVAL;
> + }
> +}
...
> +static int p3t1755_write_event_value(struct iio_dev *indio_dev,
> + const struct iio_chan_spec *chan,
> + enum iio_event_type type,
> + enum iio_event_direction dir,
> + enum iio_event_info info, int val,
> + int val2)
> +{
> + struct p3t1755_data *data = iio_priv(indio_dev);
> + unsigned int reg;
> + __be16 be;
> +
> + if (type != IIO_EV_TYPE_THRESH || info != IIO_EV_INFO_VALUE)
> + return -EINVAL;
> +
> + reg = (dir == IIO_EV_DIR_RISING) ? P3T1755_REG_HIGH_LIM :
> + P3T1755_REG_LOW_LIM;
> + if (val < -2048 || val > 2047)
> + return -ERANGE;
Logically in_range() here is better and since it's s11 type, I would
comment on this like "compare that the value is in a range of the
11-bit signed type".
> + be = cpu_to_be16((u16)((val & 0xfff) << 4));
Why casting? Why not GENMASK()?
> + return regmap_bulk_write(data->regmap, reg, &be, sizeof(be));
> +}
> +
> +static int p3t1755_trigger_one_shot(struct p3t1755_data *data)
> +{
> + unsigned int config;
> + int ret;
> +
> + mutex_lock(&data->lock);
Use guard()() from cleanup.h
> + ret = regmap_read(data->regmap, P3T1755_REG_CFGR, &config);
> + if (ret)
> + goto out;
> +
> + if (!(config & P3T1755_SHUTDOWN_BIT)) {
> + ret = -EBUSY;
> + goto out;
> + }
> +
> + config |= P3T1755_ONE_SHOT_BIT;
> + ret = regmap_write(data->regmap, P3T1755_REG_CFGR, config);
> +
> +out:
> + mutex_unlock(&data->lock);
> + return ret;
> +}
...
> + switch (iattr->address) {
> + case P3T1755_ATTR_TRIGGER_ONE_SHOT:
> + ret = kstrtobool(buf, &enable);
> + if (ret || !enable)
> + return ret ? ret : -EINVAL;
Split to two conditionals of the same level
if (ret)
return ret;
if (...)
return -E...
> + ret = p3t1755_trigger_one_shot(data);
> + return ret ?: count;
Ditto.
> + default:
> + return -EINVAL;
> + }
> + }
...
> +static IIO_DEVICE_ATTR(trigger_one_shot, 0200, NULL, p3t1755_attr_store,
> + P3T1755_ATTR_TRIGGER_ONE_SHOT);
IIO_DEVICE_ATTR_WO()
...
> +static const struct iio_event_spec p3t1755_events[] = {
> + {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_RISING,
> + .mask_separate = BIT(IIO_EV_INFO_VALUE)
Leave trailing comma.
> + },
> + {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_FALLING,
> + .mask_separate = BIT(IIO_EV_INFO_VALUE)
Ditto.
> + },
> +};
...
> +const struct p3t1755_info p3t1755_channels_info = {
> + .name = "p3t1755",
> + .channels = p3t1755_channels,
> + .num_channels = ARRAY_SIZE(p3t1755_channels)
Ditto.
> +};
...
> +static struct attribute *p3t1755_attributes[] = {
> + &iio_dev_attr_trigger_one_shot.dev_attr.attr,
> + NULL,
Remove trailing comma for the terminator entry.
> +};
...
> +int p3t1755_probe(struct device *dev, const struct p3t1755_info *chip,
> + struct regmap *regmap, bool tm_mode, int fq_bits, int irq)
> +{
> + struct p3t1755_data *data;
> + struct iio_dev *iio_dev;
> + unsigned long irq_flags;
> + int ret;
> +
> + iio_dev = devm_iio_device_alloc(dev, sizeof(*data));
> + if (!iio_dev)
> + return -ENOMEM;
> +
> + data = iio_priv(iio_dev);
> + data->dev = dev;
> + data->regmap = regmap;
> + data->tm_mode = tm_mode;
> + mutex_init(&data->lock);
devm_mutex_init()
> + iio_dev->name = chip->name;
> + iio_dev->modes = INDIO_DIRECT_MODE;
> + iio_dev->info = &p3t1755_info;
> + iio_dev->channels = chip->channels;
> + iio_dev->num_channels = chip->num_channels;
> +
> + dev_set_drvdata(dev, iio_dev);
> +
> + ret = regmap_update_bits(data->regmap, P3T1755_REG_CFGR,
> + P3T1755_TM_BIT,
> + (tm_mode ? P3T1755_TM_BIT : 0));
Too many parentheses.
> + if (ret)
> + return dev_err_probe(data->dev, ret, "Failed to update TM bit\n");
> +
> + if (fq_bits >= 0)
> + regmap_update_bits(data->regmap, P3T1755_REG_CFGR, P3T1755_FAULT_QUEUE_MASK,
> + fq_bits << P3T1755_FAULT_QUEUE_SHIFT);
> +
> + ret = devm_iio_device_register(dev, iio_dev);
> + if (ret)
> + return dev_err_probe(dev, ret, "Temperature sensor failed to register\n");
> +
> + if (irq > 0) {
> + iio_dev = dev_get_drvdata(dev);
> + data = iio_priv(iio_dev);
> + if (tm_mode)
> + irq_flags = IRQF_TRIGGER_FALLING;
> + else
> + irq_flags = (IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING);
As David said, we use firmware description for these.
> + ret = devm_request_threaded_irq(dev, irq, NULL,
> + p3t1755_irq_handler, irq_flags | IRQF_ONESHOT,
> + "p3t175x", iio_dev);
> + if (ret)
> + dev_err_probe(dev, ret, "Failed to request IRQ: %d\n", ret);
> + }
> +
> + return 0;
> +}
...
> +#include <linux/module.h>
> +#include <linux/i2c.h>
> +#include <linux/slab.h>
> +#include <linux/regmap.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/events.h>
Follow IWYU, please.
...
> +static int p3t1755_i2c_probe(struct i2c_client *client)
> +{
> + const struct p3t1755_info *chip;
> + struct regmap *regmap;
> + bool tm_mode = false;
> + int fq_bits = -1;
> + int ret;
> + u32 fq;
> +
> + regmap = devm_regmap_init_i2c(client, &p3t1755_i2c_regmap_config);
> + if (IS_ERR(regmap)) {
> + return dev_err_probe(&client->dev, PTR_ERR(regmap),
With
struct device *dev = &client->dev;
this and other code statements become neater.
> + "regmap init failed\n");
> + }
> +
> + tm_mode = device_property_read_bool(&client->dev, "nxp,interrupt-mode");
> +
> + if (!device_property_read_u32(&client->dev, "nxp,fault-queue", &fq)) {
> + fq_bits = p3t1755_fault_queue_to_bits(fq);
> + if (fq_bits < 0) {
> + return dev_err_probe(&client->dev, fq_bits,
> + "invalid nxp,fault-queue %u (1/2/4/6)\n", fq);
> + }
Why {} ?
> + }
> +
> + dev_dbg(&client->dev, "Using TM mode: %s\n",
> + tm_mode ? "Interrupt" : "Comparator");
> +
> + chip = i2c_get_match_data(client);
> +
> + dev_dbg(&client->dev, "Registering p3t175x temperature sensor");
> +
> + ret = p3t1755_probe(&client->dev, chip, regmap,
> + tm_mode, fq_bits, client->irq);
> +
> + if (ret) {
> + dev_err_probe(&client->dev, ret, "p3t175x probe failed: %d\n", ret);
> + return ret;
Again, it looks like drivers are written by two or more people. Use
consistent style everywhere.
return dev_err_probe(...);
> + }
> +
> + return 0;
> +}
...
> +++ b/drivers/iio/temperature/p3t/p3t1755_i3c.c
> +/*
> + * Both P3T1755 and P3T1750 share the same I3C PID (0x011B:0x152A),
> + * making runtime differentiation impossible, so using "p3t1755" as
> + * name in sysfs and IIO for I3C based instances.
> + */
> +static const struct i3c_device_id p3t1755_i3c_ids[] = {
> + I3C_DEVICE(0x011B, 0x152A, &p3t1755_channels_info),
> + { },
No comma for the terminator line.
> +};
> +
Redundant blank line.
> +MODULE_DEVICE_TABLE(i3c, p3t1755_i3c_ids);
> +static int p3t1755_i3c_probe(struct i3c_device *i3cdev)
> +{
> + const struct regmap_config p3t1755_i3c_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + };
Why not in the same way as in i²c driver?
> + const struct i3c_device_id *id = i3c_device_match_id(i3cdev, p3t1755_i3c_ids);
> + const struct p3t1755_info *chip;
> + struct device *dev = &i3cdev->dev;
> + struct i3c_ibi_setup ibi_setup;
> + struct regmap *regmap;
> + bool tm_mode = false;
> + int fq_bits = -1;
> + int ret;
> + u32 fq;
> + chip = id ? id->data : NULL;
Can i3c code gain or may already have the analogue of
device_get_match_data() as i²c has?
> + regmap = devm_regmap_init_i3c(i3cdev, &p3t1755_i3c_regmap_config);
> + if (IS_ERR(regmap)) {
> + return dev_err_probe(&i3cdev->dev, PTR_ERR(regmap),
> + "Failed to register I3C regmap %ld\n", PTR_ERR(regmap));
> + }
> +
> + tm_mode = device_property_read_bool(dev, "nxp,interrupt-mode");
> + if (!device_property_read_u32(dev, "nxp,fault-queue", &fq)) {
> + fq_bits = p3t1755_fault_queue_to_bits(fq);
> + if (fq_bits < 0) {
> + return dev_err_probe(&i3cdev->dev, fq_bits,
> + "invalid nxp,fault-queue %u (1/2/4/6)\n", fq);
> + }
> + }
Isn't it the same as in i²c? Make it part of the core probe instead.
> + dev_dbg(&i3cdev->dev, "Using TM mode: %s\n", tm_mode ? "Interrupt" : "Comparator");
Ditto. Also why is this message detached from the above
device_property_read_bool()?
> + ret = p3t1755_probe(dev, chip, regmap, tm_mode, fq_bits, 0);
> + if (ret)
> + return dev_err_probe(dev, ret, "p3t175x probe failed: %d\n", ret);
> +
> + if (!tm_mode) {
> + dev_warn(&i3cdev->dev, "IBI not supported in comparator mode, skipping IBI registration\n");
> + return 0;
> + }
> +
> + ibi_setup = (struct i3c_ibi_setup) {
> + .handler = p3t1755_ibi_handler,
> + .num_slots = 4,
> + .max_payload_len = 0,
> + };
> +
> + ret = i3c_device_request_ibi(i3cdev, &ibi_setup);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to request IBI\n");
> +
> + ret = i3c_device_enable_ibi(i3cdev);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to enable IBI\n");
> +
> + ret = devm_add_action_or_reset(dev, p3t1755_disable_ibi, i3cdev);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to register IBI disable action\n");
> +
> + ret = devm_add_action_or_reset(dev, p3t1755_free_ibi, i3cdev);
> + if (ret)
> + return dev_err_probe(dev, ret, "Failed to register IBI free action\n");
> + dev_dbg(&i3cdev->dev, "IBI successfully registered\n");
Noise, remove.
> + return 0;
> +}
--
With Best Regards,
Andy Shevchenko
Powered by blists - more mailing lists