On Tue, 30 Apr 2024 14:05:35 +0200
Dimitri Fedrau <dima.fedrau@xxxxxxxxx> 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@xxxxxxxxx>
> ---
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@xxxxxxxxx>
> */
>
> +#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@xxxxxxxxx>");
> MODULE_AUTHOR("Andrew Hepp <andrew.hepp@xxxxxxxxx>");
> MODULE_DESCRIPTION("Microchip MCP9600 thermocouple EMF converter driver");
> MODULE_LICENSE("GPL");
