Message-ID: <20251004155507.44798db5@jic23-huawei>
Date: Sat, 4 Oct 2025 15:55:07 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Lakshay Piplani <lakshay.piplani@....com>
Cc: linux-kernel@...r.kernel.org, linux-iio@...r.kernel.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, jonathan.cameron@...wei.com,
 vikash.bansal@....com, priyanka.jain@....com, shashank.rebbapragada@....com
Subject: Re: [PATCH v3 2/2] iio: temperature: Add driver for NXP P3T175x
 temperature sensor

On Mon, 29 Sep 2025 15:15:43 +0530
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.
> 
> Signed-off-by: Lakshay Piplani <lakshay.piplani@....com>
Hi Lakshay,

Please add a cover letter. I would have replied to that if there was one.

After some recent feedback on hwmon vs IIO sensors and the fact we do
indeed seem to be slowly drifting wrt to the boundary for what I let into IIO
I'm asking people to +CC the hwmon maintainers and provide an explanation of why
they think a particular device should be supported with an IIO driver rather than
a hwmon one. 

So +CC Guenter and Jean,

Sorry I didn't raise this earlier. I've clearly not been paying quite enough attention
to this.  Had my head down in code reviews rather than looking at the bigger picture.

I'm not immediately spotting characteristics that would clearly justify this being
in IIO.

Review comments inline.

Thanks,

Jonathan


> ---
> V2 -> V3: Changes since V2:
>           - Dropped nxp,interrupt-mode and nxp,fault-queue from driver and YAML (not suitable for DT)
>           - Removed trigger_one_shot sysfs attribute and its ABI doc
>           - Applied IWYU principle: cleaned up unused headers
>           - Fixed sampling frequency handling
>           - Removed dev_err/dev_dbg statements wherever not necessary
> V1 -> V2: Changes since V1:
>           - Added endian-safe handling for register read (__be16 conversion)
>           - Replaced manual bit masking with FIELD_GET bit extraction
>           - Dropped sysfs attributes for fault queue length and thermostat mode (comparator or interrupt)
>           - Added ABI doc: Documentation/ABI/testing/sysfs-bus-iio-temperature-p3t1755 describing
>             trigger_one_shot attribute
>           - Updated Kconfig to allow building both I2C and I3C drivers simultaneously
>           - I3C: switched to device_property_* from of_property_*
>           - Added devm_add_action_or_reset() for IBI disable/free
> 
>  drivers/iio/temperature/Kconfig            |   2 +
>  drivers/iio/temperature/p3t/Kconfig        |  28 ++
>  drivers/iio/temperature/p3t/Makefile       |   5 +
>  drivers/iio/temperature/p3t/p3t1755.h      |  45 +++
>  drivers/iio/temperature/p3t/p3t1755_core.c | 362 +++++++++++++++++++++
>  drivers/iio/temperature/p3t/p3t1755_i2c.c  |  68 ++++
>  drivers/iio/temperature/p3t/p3t1755_i3c.c  | 108 ++++++
>  7 files changed, 618 insertions(+)
>  create mode 100644 drivers/iio/temperature/p3t/Kconfig
>  create mode 100644 drivers/iio/temperature/p3t/Makefile
>  create mode 100644 drivers/iio/temperature/p3t/p3t1755.h
>  create mode 100644 drivers/iio/temperature/p3t/p3t1755_core.c
>  create mode 100644 drivers/iio/temperature/p3t/p3t1755_i2c.c
>  create mode 100644 drivers/iio/temperature/p3t/p3t1755_i3c.c
> 

> diff --git a/drivers/iio/temperature/p3t/p3t1755.h b/drivers/iio/temperature/p3t/p3t1755.h
> new file mode 100644
> index 000000000000..1dc0e37322c6
> --- /dev/null
> +++ b/drivers/iio/temperature/p3t/p3t1755.h

> diff --git a/drivers/iio/temperature/p3t/p3t1755_core.c b/drivers/iio/temperature/p3t/p3t1755_core.c
> new file mode 100644
> index 000000000000..61486eb0e265
> --- /dev/null
> +++ b/drivers/iio/temperature/p3t/p3t1755_core.c


> +
> +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)
> +			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)
> +			return ret;
> +
> +		*val = !FIELD_GET(P3T1755_SHUTDOWN_BIT, cfgr);
> +
> +		return IIO_VAL_INT;
> +
> +	case IIO_CHAN_INFO_SAMP_FREQ: {
> +		unsigned int freq_uhz;
> +		u8 sel;
> +
> +		ret = regmap_read(data->regmap, P3T1755_REG_CFGR, &cfgr);
> +		if (ret)
> +			return ret;
> +
> +		sel = FIELD_GET(P3T1755_CONVERSION_TIME_BITS, cfgr);
> +		if (sel >= ARRAY_SIZE(p3t1755_samp_freqs_uhz))
> +			return -EINVAL;
> +
> +		freq_uhz = p3t1755_samp_freqs_uhz[sel];
> +
> +		*val = freq_uhz / 1000000;
> +		*val2 = freq_uhz % 1000000;

MICRO (see below).

> +
> +		return IIO_VAL_INT_PLUS_MICRO;
> +	}
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int p3t1755_write_raw(struct iio_dev *indio_dev,
> +			     struct iio_chan_spec const *chan, int val,
> +			     int val2, long mask)
> +{
> +	struct p3t1755_data *data = iio_priv(indio_dev);
> +	int ret;
> +
> +	switch (mask) {
> +	case IIO_CHAN_INFO_ENABLE:
> +		ret = regmap_update_bits(data->regmap, P3T1755_REG_CFGR,
> +					 P3T1755_SHUTDOWN_BIT,
> +					 val == 0 ? P3T1755_SHUTDOWN_BIT : 0);
> +		if (ret)
> +			return ret;
> +
> +		return 0;
> +	case IIO_CHAN_INFO_SAMP_FREQ: {
> +		unsigned int i;
> +		u32 regbits;
> +		u64 input_uhz;
> +
> +		input_uhz = (u64)val * 1000000 + val2;

MICRO perhaps as that's on the border of how many zeros are easy to count.

> +
> +		for (i = 0; i < ARRAY_SIZE(p3t1755_samp_freqs_uhz); i++) {
> +			if (p3t1755_samp_freqs_uhz[i] == input_uhz)
> +				break;
> +		}
> +
> +		if (i == ARRAY_SIZE(p3t1755_samp_freqs_uhz))
> +			return -EINVAL;
> +
> +		regbits = FIELD_PREP(P3T1755_CONVERSION_TIME_BITS, i);
> +
> +		return regmap_update_bits(data->regmap, P3T1755_REG_CFGR,
> +					  P3T1755_CONVERSION_TIME_BITS,
> +					  regbits);
> +	}
> +	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;
> +	int raw;
> +
> +	if (type != IIO_EV_TYPE_THRESH || info != IIO_EV_INFO_VALUE)
> +		return -EINVAL;
> +
> +	if (val2)
> +		return -EINVAL;
> +
> +	reg = (dir == IIO_EV_DIR_RISING) ? P3T1755_REG_HIGH_LIM :
> +	       P3T1755_REG_LOW_LIM;
> +
> +	raw = DIV_ROUND_CLOSEST(val * 2, 125);
> +
> +	if (raw < -2048 || raw > 2047)
> +		return -ERANGE;
> +
> +	be = cpu_to_be16((u16)(raw << 4));
> +
> +	return regmap_raw_write(data->regmap, reg, &be, sizeof(be));

Why is raw write appropriate here rather than bulk_write ?
Perhaps a comment would be a good idea for anyone wondering in future.

> +}

> +
> +MODULE_AUTHOR("Lakshay Piplani <lakshay.piplani@....com>");
> +MODULE_DESCRIPTION("NXP P3T175x Driver");

As below wrt to wild cards.

> +MODULE_LICENSE("GPL");
> diff --git a/drivers/iio/temperature/p3t/p3t1755_i2c.c b/drivers/iio/temperature/p3t/p3t1755_i2c.c
> new file mode 100644
> index 000000000000..f5d7799f091c
> --- /dev/null
> +++ b/drivers/iio/temperature/p3t/p3t1755_i2c.c

> +static int p3t1755_i2c_probe(struct i2c_client *client)
> +{
> +	struct device *dev = &client->dev;
> +	const struct p3t1755_info *chip;
> +	struct regmap *regmap;
> +	int ret;
> +
> +	regmap = devm_regmap_init_i2c(client, &p3t1755_i2c_regmap_config);
> +	if (IS_ERR(regmap))
> +		return dev_err_probe(dev, PTR_ERR(regmap),
> +				     "regmap init failed\n");
> +
> +	chip = i2c_get_match_data(client);
> +
> +	ret = p3t1755_probe(dev, chip, regmap, client->irq);
> +
Drop this blank line. We want the error handling closely coupled with the call.

> +	if (ret)
> +		return dev_err_probe(dev, ret, "p3t175x probe failed: %d\n", ret);
> +
> +	return 0;
> +}
> +
> +static struct i2c_driver p3t1755_driver = {
> +	.driver = {
> +		.name = "p3t175x_i2c",

Pick a part number rather than using a wild card. Wildcards often down withstand
the desire of marketing departments to fill in the holes.

> +		.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");

I'd use a part number rather than wild card here as well.

> +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..df031280e08d
> --- /dev/null
> +++ b/drivers/iio/temperature/p3t/p3t1755_i3c.c
> @@ -0,0 +1,108 @@

...

> +
> +static void p3t1755_disable_ibi(void *data)
> +{
> +	i3c_device_disable_ibi((struct i3c_device *)data);

As below - unnecessary cast.

> +}
> +
> +static void p3t1755_free_ibi(void *data)
> +{
> +	i3c_device_free_ibi((struct i3c_device *)data);

That cast isn't needed.  Casting from void * to any other pointer is implicitly allowed
by the c spec.  So just pass data directly.

> +}
> +
> +static int p3t1755_i3c_probe(struct i3c_device *i3cdev)
> +{
> +	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;
> +	int ret;
> +
> +	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));
> +
> +	ret = p3t1755_probe(dev, chip, regmap, 0);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "p3t175x probe failed: %d\n", ret);
> +
> +	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 = 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");

As below.

> +
> +	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)

Don't bother with error prints for devm_add_action_or_reset().
The only failure path returns -ENOMEM which doesn't result in a print in
dev_err_probe() because the expectation is that it is an unlikely condition
which results in plenty of prints anyway.
So simple
		return ret;
appropriate for these calls.


> +		return dev_err_probe(dev, ret, "Failed to register IBI disable action\n");
> +
> +	return 0;
> +}

Powered by blists - more mailing lists