Date: Sun, 5 May 2024 18:47:00 +0100
From: Jonathan Cameron <jic23@...nel.org>
To: Dimitri Fedrau <dima.fedrau@...il.com>
Cc: Lars-Peter Clausen <lars@...afoo.de>, Andrew Hepp
 <andrew.hepp@...pp.dev>, Marcelo Schmitt <marcelo.schmitt1@...il.com>,
 linux-iio@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/5] iio: temperature: mcp9600: add threshold events
 support

On Tue, 30 Apr 2024 14:05:35 +0200
Dimitri Fedrau <dima.fedrau@...il.com> wrote:

> The device has four programmable temperature alert outputs which can be
> used to monitor hot or cold-junction temperatures and detect falling and
> rising temperatures. It supports up to 255 degree celsius programmable
> hysteresis. Each alert can be individually configured by setting following
> options in the associated alert configuration register:
>   - monitor hot or cold junction temperature
>   - monitor rising or falling temperature
>   - set comparator or interrupt mode
>   - set output polarity
>   - enable alert
> 
> This patch binds alert outputs to iio events:
>   - alert1: hot junction, rising temperature
>   - alert2: hot junction, falling temperature
>   - alert3: cold junction, rising temperature
>   - alert4: cold junction, falling temperature
> 
> All outputs are set in comparator mode and polarity depends on interrupt
> configuration.
> 
> Signed-off-by: Dimitri Fedrau <dima.fedrau@...il.com>
> ---
Various comments inline.

Jonathan

>  drivers/iio/temperature/mcp9600.c | 358 +++++++++++++++++++++++++++++-
>  1 file changed, 354 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
> index cb1c1c1c361d..f7e1b4e3253d 100644
> --- a/drivers/iio/temperature/mcp9600.c
> +++ b/drivers/iio/temperature/mcp9600.c
> @@ -6,21 +6,80 @@
>   * Author: <andrew.hepp@...pp.dev>
>   */
>  
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
>  #include <linux/err.h>
>  #include <linux/i2c.h>
>  #include <linux/init.h>
> +#include <linux/math.h>
> +#include <linux/minmax.h>
>  #include <linux/mod_devicetable.h>
>  #include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/units.h>
>  
> +#include <linux/iio/events.h>
>  #include <linux/iio/iio.h>
>  
>  /* MCP9600 registers */
> -#define MCP9600_HOT_JUNCTION 0x0

As below. Reformating in a precursor patch. I wouldn't necessarily bother
though as aligning defines is usually more effort than it is worth over time.

> -#define MCP9600_COLD_JUNCTION 0x2
> -#define MCP9600_DEVICE_ID 0x20
> +#define MCP9600_HOT_JUNCTION		0x0
> +#define MCP9600_COLD_JUNCTION		0x2
> +#define MCP9600_STATUS			0x4
> +#define MCP9600_STATUS_ALERT(x)		BIT(x)
> +#define MCP9600_ALERT_CFG1		0x8
> +#define MCP9600_ALERT_CFG(x)		(MCP9600_ALERT_CFG1 + (x - 1))
> +#define MCP9600_ALERT_CFG_ENABLE	BIT(0)
> +#define MCP9600_ALERT_CFG_ACTIVE_HIGH	BIT(2)
> +#define MCP9600_ALERT_CFG_FALLING	BIT(3)
> +#define MCP9600_ALERT_CFG_COLD_JUNCTION	BIT(4)
> +#define MCP9600_ALERT_HYSTERESIS1	0xc
> +#define MCP9600_ALERT_HYSTERESIS(x)	(MCP9600_ALERT_HYSTERESIS1 + (x - 1))
> +#define MCP9600_ALERT_LIMIT1		0x10
> +#define MCP9600_ALERT_LIMIT(x)		(MCP9600_ALERT_LIMIT1 + (x - 1))
> +
> +#define MCP9600_DEVICE_ID		0x20
>  
>  /* MCP9600 device id value */
> -#define MCP9600_DEVICE_ID_MCP9600 0x40
> +#define MCP9600_DEVICE_ID_MCP9600	0x40

If you want to reformatting existing lines, do it in a precursor patch - not
buried in here.

>  
>  struct mcp9600_data {
>  	struct i2c_client *client;
> +	struct mutex lock[MCP9600_ALERT_COUNT];

All locks need documentation.  What data is this protecting?

> +	int irq[MCP9600_ALERT_COUNT];
>  };
>  
>  static int mcp9600_read(struct mcp9600_data *data,
> @@ -83,10 +148,292 @@ static int mcp9600_read_raw(struct iio_dev *indio_dev,
>  	}
>  }
>  
> +static int mcp9600_get_alert_index(int channel2, enum iio_event_direction dir)
> +{
> +	switch (channel2) {
> +	case IIO_MOD_TEMP_OBJECT:
> +		if (dir == IIO_EV_DIR_RISING)
> +			return MCP9600_ALERT1;
> +		else
> +			return MCP9600_ALERT2;
> +	case IIO_MOD_TEMP_AMBIENT:
> +		if (dir == IIO_EV_DIR_RISING)
> +			return MCP9600_ALERT3;
> +		else
> +			return MCP9600_ALERT4;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
> +static int mcp9600_read_event_config(struct iio_dev *indio_dev,
> +				     const struct iio_chan_spec *chan,
> +				     enum iio_event_type type,
> +				     enum iio_event_direction dir)
> +{
> +	struct mcp9600_data *data = iio_priv(indio_dev);
> +	struct i2c_client *client = data->client;
> +	int i, ret;
> +
> +	i = mcp9600_get_alert_index(chan->channel2, dir);
> +	if (i < 0)
> +		return i;
> +
> +	ret = i2c_smbus_read_byte_data(client, MCP9600_ALERT_CFG(i + 1));
> +	if (ret < 0)
> +		return ret;
> +
> +	return (ret & MCP9600_ALERT_CFG_ENABLE);

FIELD_GET() even if it happens to be bit(0) as then we don't have to go
check that's the case.

> +}
> +
> +static int mcp9600_write_event_config(struct iio_dev *indio_dev,
> +				      const struct iio_chan_spec *chan,
> +				      enum iio_event_type type,
> +				      enum iio_event_direction dir,
> +				      int state)
> +{
> +	struct mcp9600_data *data = iio_priv(indio_dev);
> +	struct i2c_client *client = data->client;
> +	int i, ret;
> +
> +	i = mcp9600_get_alert_index(chan->channel2, dir);
> +	if (i < 0)
> +		return i;
> +
> +	ret = i2c_smbus_read_byte_data(client, MCP9600_ALERT_CFG(i + 1));
> +	if (ret < 0)
> +		return ret;
> +
> +	if (state)
> +		ret |= MCP9600_ALERT_CFG_ENABLE;
> +	else
> +		ret &= ~MCP9600_ALERT_CFG_ENABLE;
> +
> +	return i2c_smbus_write_byte_data(client, MCP9600_ALERT_CFG(i + 1), ret);

A read modify write cycle like this normally needs some locking to ensure another
access didn't change the other bits in the register.


> +}
> +
> +static int mcp9600_read_thresh(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 mcp9600_data *data = iio_priv(indio_dev);
> +	struct i2c_client *client = data->client;
> +	s32 ret;
> +	int i;
> +
> +	i = mcp9600_get_alert_index(chan->channel2, dir);
> +	if (i < 0)
> +		return i;
> +
> +	guard(mutex)(&data->lock[i]);
> +	ret = i2c_smbus_read_word_swapped(client, MCP9600_ALERT_LIMIT(i + 1));
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * Temperature is stored in two’s complement format in bits(15:2),
> +	 * LSB is 0.25 degree celsius.
> +	 */
> +	*val = sign_extend32(ret, 15) >> 2;
Use sign_extend32(FIELD_GET(...), 13)
So which bits are extracted is obvious in the code.

> +	*val2 = 4;
> +	if (info == IIO_EV_INFO_VALUE)
> +		return IIO_VAL_FRACTIONAL;
> +
> +	ret = i2c_smbus_read_byte_data(client, MCP9600_ALERT_HYSTERESIS(i + 1));
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * Hysteresis is stored as offset which is not signed, therefore we have
> +	 * to include directions when calculating the real hysteresis value.
> +	 */
> +	if (dir == IIO_EV_DIR_RISING)
> +		*val -= (*val2 * ret);
> +	else
> +		*val += (*val2 * ret);

I don't follow this maths.  Hysteresis is an unsigned offset.  Maybe some confusion
over the ABI?  

> +
> +	return IIO_VAL_FRACTIONAL;
> +}
> +
> +static int mcp9600_write_thresh(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 mcp9600_data *data = iio_priv(indio_dev);
> +	struct i2c_client *client = data->client;
> +	int s_val, s_thresh, i;
> +	s16 thresh;
> +	s32 ret;
> +	u8 hyst;
> +
> +	/* Scale value to include decimal part into calculations */
> +	s_val = (val < 0) ? ((val * (int)MICRO) - val2) :
> +			    ((val * (int)MICRO) + val2);
> +
> +	/* Hot junction temperature range is from –200 to 1800 degree celsius */
> +	if (chan->channel2 == IIO_MOD_TEMP_OBJECT &&
> +	   (s_val < (MCP9600_MIN_TEMP_HOT_JUNCTION * (int)MICRO) ||
> +	    s_val > (MCP9600_MAX_TEMP_HOT_JUNCTION * (int)MICRO)))

Why the casts?

> +		return -EINVAL;
> +
> +	/* Cold junction temperature range is from –40 to 125 degree celsius */
> +	if (chan->channel2 == IIO_MOD_TEMP_AMBIENT &&
> +	   (s_val < (MCP9600_MIN_TEMP_COLD_JUNCTION * (int)MICRO) ||
> +	    s_val > (MCP9600_MAX_TEMP_COLD_JUNCTION * (int)MICRO)))
> +		return -EINVAL;
> +
> +	i = mcp9600_get_alert_index(chan->channel2, dir);
> +	if (i < 0)
> +		return i;
> +
> +	guard(mutex)(&data->lock[i]);
> +	if (info == IIO_EV_INFO_VALUE) {

I would use a switch statement so it is obvious what each case is.

> +		/*
> +		 * Shift length 4 bits = 2(15:2) + 2(0.25 LSB), temperature is
> +		 * stored in two’s complement format.
> +		 */
> +		thresh = (s16)(s_val / (int)(MICRO >> 4));
> +		return i2c_smbus_write_word_swapped(client,
> +						    MCP9600_ALERT_LIMIT(i + 1),
> +						    thresh);
> +	}
> +
> +	/* Read out threshold, hysteresis is stored as offset */
> +	ret = i2c_smbus_read_word_swapped(client, MCP9600_ALERT_LIMIT(i + 1));
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Shift length 4 bits = 2(15:2) + 2(0.25 LSB), see above. */
> +	s_thresh = sign_extend32(ret, 15) * (int)(MICRO >> 4);
> +
> +	/*
> +	 * Hysteresis is stored as offset, for rising temperatures, the
> +	 * hysteresis range is below the alert limit where, as for falling
> +	 * temperatures, the hysteresis range is above the alert limit.
> +	 */
> +	hyst = min(255, abs(s_thresh - s_val) / MICRO);
> +
> +	return i2c_smbus_write_byte_data(client,
> +					 MCP9600_ALERT_HYSTERESIS(i + 1),
> +					 hyst);
> +}
> +
>  static const struct iio_info mcp9600_info = {
>  	.read_raw = mcp9600_read_raw,
> +	.read_event_config = mcp9600_read_event_config,
> +	.write_event_config = mcp9600_write_event_config,
> +	.read_event_value = mcp9600_read_thresh,
> +	.write_event_value = mcp9600_write_thresh,
>  };
>  
> +static irqreturn_t mcp9600_alert_handler(int irq, void *private)
> +{
> +	struct iio_dev *indio_dev = private;
> +	struct mcp9600_data *data = iio_priv(indio_dev);
> +	enum iio_event_direction dir;
> +	enum iio_modifier mod;
> +	int i, ret;
> +
> +	for (i = 0; i < MCP9600_ALERT_COUNT; i++) {
> +		if (data->irq[i] == irq)

This search for a match is a little messy. I'd be tempted
to wrap the generic handler in a per instance interrupt handler
(so have 4 functions) and thus move this matching to the place
where they are registered, not the interrupt handler.

There isn't a lot of shared code in here so you may be better
off just having 4 separate interrupt handler implementations.

> +			break;
> +	}
> +
> +	if (i >= MCP9600_ALERT_COUNT)
> +		return IRQ_NONE;
> +
> +	ret = i2c_smbus_read_byte_data(data->client, MCP9600_STATUS);
> +	if (ret < 0)
> +		return IRQ_HANDLED;
> +
> +	switch (ret & MCP9600_STATUS_ALERT(i)) {
> +	case 0:
> +		return IRQ_NONE;
> +	case MCP9600_STATUS_ALERT(MCP9600_ALERT1):
> +		mod = IIO_MOD_TEMP_OBJECT;
> +		dir = IIO_EV_DIR_RISING;
> +		break;
> +	case MCP9600_STATUS_ALERT(MCP9600_ALERT2):
> +		mod = IIO_MOD_TEMP_OBJECT;
> +		dir = IIO_EV_DIR_FALLING;
> +		break;
> +	case MCP9600_STATUS_ALERT(MCP9600_ALERT3):
> +		mod = IIO_MOD_TEMP_AMBIENT;
> +		dir = IIO_EV_DIR_RISING;
> +		break;
> +	case MCP9600_STATUS_ALERT(MCP9600_ALERT4):
> +		mod = IIO_MOD_TEMP_AMBIENT;
> +		dir = IIO_EV_DIR_FALLING;
> +		break;
> +	default:
> +		return IRQ_HANDLED;
> +	}
> +
> +	iio_push_event(indio_dev,
> +		       IIO_MOD_EVENT_CODE(IIO_TEMP, 0, mod,
> +					  IIO_EV_TYPE_THRESH, dir),
> +		       iio_get_time_ns(indio_dev));
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int mcp9600_probe_alerts(struct iio_dev *indio_dev)
> +{
> +	struct mcp9600_data *data = iio_priv(indio_dev);
> +	struct i2c_client *client = data->client;
> +	struct device *dev = &client->dev;
> +	struct fwnode_handle *fwnode = dev_fwnode(dev);
> +	unsigned int irq_type;
> +	int ret, irq, i;
> +	u8 val;
> +
> +	/*
> +	 * alert1: hot junction, rising temperature
> +	 * alert2: hot junction, falling temperature
> +	 * alert3: cold junction, rising temperature
> +	 * alert4: cold junction, falling temperature
> +	 */
> +	for (i = 0; i < MCP9600_ALERT_COUNT; i++) {
> +		data->irq[i] = 0;

All of data is zeroed already so this should not be needed.

> +		mutex_init(&data->lock[i]);

Why per interrupt locks?  Seems unlikely to be a big problem
to share one.

> +		irq = fwnode_irq_get_byname(fwnode, mcp9600_alert_name[i]);
> +		if (irq <= 0)
> +			continue;
> +
> +		val = 0;
> +		irq_type = irq_get_trigger_type(irq);
> +		if (irq_type == IRQ_TYPE_EDGE_RISING)
> +			val |= MCP9600_ALERT_CFG_ACTIVE_HIGH;
> +
> +		if (i == MCP9600_ALERT2 || i == MCP9600_ALERT4)
> +			val |= MCP9600_ALERT_CFG_FALLING;
> +
> +		if (i == MCP9600_ALERT3 || i == MCP9600_ALERT4)
> +			val |= MCP9600_ALERT_CFG_COLD_JUNCTION;
> +
> +		ret = i2c_smbus_write_byte_data(client,
> +						MCP9600_ALERT_CFG(i + 1),
> +						val);
> +		if (ret < 0)
> +			return ret;
> +
> +		ret = devm_request_threaded_irq(dev, irq, NULL,
> +						mcp9600_alert_handler,
> +						IRQF_ONESHOT, "mcp9600",
> +						indio_dev);
> +		if (ret)
> +			return ret;
> +
> +		data->irq[i] = irq;
> +	}
> +
> +	return 0;
> +}
> +
>  static int mcp9600_probe(struct i2c_client *client)
>  {
>  	struct device *dev = &client->dev;
> @@ -109,6 +456,8 @@ static int mcp9600_probe(struct i2c_client *client)
>  	data = iio_priv(indio_dev);
>  	data->client = client;
>  
> +	mcp9600_probe_alerts(indio_dev);

Why no error check? 

> +
>  	indio_dev->info = &mcp9600_info;
>  	indio_dev->name = "mcp9600";
>  	indio_dev->modes = INDIO_DIRECT_MODE;
> @@ -140,6 +489,7 @@ static struct i2c_driver mcp9600_driver = {
>  };
>  module_i2c_driver(mcp9600_driver);
>  
> +MODULE_AUTHOR("Dimitri Fedrau <dima.fedrau@...il.com>");
>  MODULE_AUTHOR("Andrew Hepp <andrew.hepp@...pp.dev>");
>  MODULE_DESCRIPTION("Microchip MCP9600 thermocouple EMF converter driver");
>  MODULE_LICENSE("GPL");

Powered by blists - more mailing lists