Message-ID: <72c3ae7d-cb54-4a1b-a27a-9e673ffaddcc@kernel.org>
Date: Sun, 31 Aug 2025 18:46:32 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: Lakshay Piplani <lakshay.piplani@....com>, 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
Cc: 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 27/08/2025 12:31, Lakshay Piplani 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.
> 


...

> +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;
> +
> +	be = cpu_to_be16((u16)((val & 0xfff) << 4));
> +
> +	return regmap_bulk_write(data->regmap, reg, &be, sizeof(be));

Now I wonder why regmap does not handle your data format?

> +}
> +
> +static int p3t1755_trigger_one_shot(struct p3t1755_data *data)
> +{
> +	unsigned int config;
> +	int ret;
> +
> +	mutex_lock(&data->lock);

Just use guard.

> +
> +	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;
> +}


...


> +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),
> +				     "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);
> +			}
> +	}
> +
> +	dev_dbg(&client->dev, "Using TM mode: %s\n",
> +		tm_mode ? "Interrupt" : "Comparator");

Pretty useless, static coming from DT :/

> +
> +	chip = i2c_get_match_data(client);
> +
> +	dev_dbg(&client->dev, "Registering p3t175x temperature sensor");

Drop

> +
> +	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;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct i2c_driver p3t1755_driver = {
> +	.driver = {
> +		.name = "p3t175x_i2c",
> +		.of_match_table = p3t1755_i2c_of_match,
> +	},
> +	.probe = p3t1755_i2c_probe,
> +	.id_table = p3t1755_i2c_id_table,
> +};
> +module_i2c_driver(p3t1755_driver);
> +
> +MODULE_AUTHOR("Lakshay Piplani <lakshay.piplani@....com>");
> +MODULE_DESCRIPTION("NXP P3T175x I2C Driver");
> +MODULE_LICENSE("GPL");
> +MODULE_IMPORT_NS(IIO_P3T1755);
> diff --git a/drivers/iio/temperature/p3t/p3t1755_i3c.c b/drivers/iio/temperature/p3t/p3t1755_i3c.c
> new file mode 100644
> index 000000000000..9f61544b2eb6
> --- /dev/null
> +++ b/drivers/iio/temperature/p3t/p3t1755_i3c.c
> @@ -0,0 +1,133 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * NXP P3T175x Temperature Sensor Driver
> + *
> + * Copyright 2025 NXP
> + */
> +#include <linux/mod_devicetable.h>
> +#include <linux/module.h>
> +#include <linux/i3c/device.h>
> +#include <linux/i3c/master.h>
> +#include <linux/slab.h>
> +#include <linux/regmap.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/events.h>
> +
> +#include "p3t1755.h"
> +
> +static void p3t1755_ibi_handler(struct i3c_device *dev,
> +				const struct i3c_ibi_payload *payload)
> +{
> +	struct iio_dev *indio_dev = dev_get_drvdata(&dev->dev);
> +
> +	p3t1755_push_thresh_event(indio_dev);
> +}
> +
> +/*
> + * 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),
> +	{ },
> +};
> +
> +MODULE_DEVICE_TABLE(i3c, p3t1755_i3c_ids);
> +
> +static void p3t1755_disable_ibi(void *data)
> +{
> +	i3c_device_disable_ibi((struct i3c_device *)data);
> +}
> +
> +static void p3t1755_free_ibi(void *data)
> +{
> +	i3c_device_free_ibi((struct i3c_device *)data);
> +}
> +
> +static int p3t1755_i3c_probe(struct i3c_device *i3cdev)
> +{
> +	const struct regmap_config p3t1755_i3c_regmap_config = {
> +	.reg_bits = 8,
> +	.val_bits = 8,
> +	};
> +
> +	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;
> +
> +	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));
> +	}

No need for {}

> +
> +	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);
> +		}
> +	}
> +
> +	dev_dbg(&i3cdev->dev, "Using TM mode: %s\n", tm_mode ? "Interrupt" : "Comparator");
> +
> +	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");

You really should not need this. You already have one probe debug.


Best regards,
Krzysztof

Powered by blists - more mailing lists