Am Sun, May 19, 2024 at 05:42:48PM +0100 schrieb Jonathan Cameron: > On Fri, 17 May 2024 10:10:50 +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> > Hi Dmitri Hi Jonathan, > Please make sure to address all questions in earlier reviews, either by > changing the code, or directly answering the question. > I did, see: https://lore.kernel.org/linux-iio/20240509204559.GB3614@debian/T/#u or did I miss anything ? I'm a little bit confused. > The hysteresis handling in here is completely different from normal > and the diagrams in figure 5-10 suggest it should not be. > > Your thresholds should not include hysteresis at all as it has nothing to > do with event triggering. The hysteresis is presented to userspace so it > knows when a 'reset' of event detection logic occurs. It is expressed > as an offset (in the obvious direction) from the current threshold. > > It is always positive as negative hysteresis would be very odd. It's just > magnitude of how far back beyond the threshold the signal must go for > a reset of the signal detection logic to occur, allowing new transitions etc. > > As long as you are using an edge interrupt that just means you won't get > another interrupt until getting well away from what triggered the interrupt > last time. > You are right. Thanks a lot for your explanation. I just didn't know it, assumed the hysteresis is represented same way as the threshold. > Jonathan > > > --- > > drivers/iio/temperature/mcp9600.c | 389 ++++++++++++++++++++++++++++++ > > 1 file changed, 389 insertions(+) > > > > diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c > > index 22451d1d9e1f..91d811fe9371 100644 > > --- a/drivers/iio/temperature/mcp9600.c > > +++ b/drivers/iio/temperature/mcp9600.c > > @@ -6,12 +6,21 @@ > > * Author: <andrew.hepp@xxxxxxxxx> > > */ > > > > +#include <linux/bitfield.h> > > +#include <linux/bitops.h> > > +#include <linux/bits.h> > > #include <linux/err.h> > > #include <linux/i2c.h> > > #include <linux/init.h> > > +#include <linux/interrupt.h> > > +#include <linux/irq.h> > > +#include <linux/math.h> > > +#include <linux/minmax.h> > > #include <linux/mod_devicetable.h> > > #include <linux/module.h> > > +#include <linux/mutex.h> > > > > +#include <linux/iio/events.h> > > #include <linux/iio/iio.h> > > > > #define MCP9600_CHAN_HOT_JUNCTION 0 > > @@ -20,11 +29,65 @@ > > /* MCP9600 registers */ > > #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_ALERT_LIMIT_MASK GENMASK(15, 2) > > #define MCP9600_DEVICE_ID 0x20 > > > > /* MCP9600 device id value */ > > #define MCP9600_DEVICE_ID_MCP9600 0x40 > > > > +#define MCP9600_ALERT_COUNT 4 > > + > > +#define MCP9600_TEMP_SCALE_NUM 1000000 > > MICRO or just use that inline. > > > + > > +#define MCP9600_MIN_TEMP_HOT_JUNCTION -200 > > +#define MCP9600_MAX_TEMP_HOT_JUNCTION 1800 > Give these units in the naming and you can include the * MICRO here. > e.g. > #define MCP9600_MIN_TEMP_HOT_JUNC_MICROCELCIUS -200 * MICRO > etc > Ok. > > > + > > +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->channel, dir); > > + guard(mutex)(&data->lock); > > + 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(FIELD_GET(MCP9600_ALERT_LIMIT_MASK, ret), 13); > > + *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 unsigned offset from threshold. The alert > > + * direction bit in the alert configuration register defines whether the > > + * value is below or above the corresponding threshold. > > I'm still very very confused by this. I raised a question in the first review > and you didn't provide more information. > This is not how hysteresis is normally defined. It should not affect the > threshold at all, but instead affect when a reset occurs of the threshold detection > logic. It also does not correspond to the diagrams in Figure 5-10 which look > exactly like normal hysteresis controls. > You are right. I got it wrong here. After your explanation and having a look at https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-bus-iio it should be easy to fix this. Thanks again. > > > + */ > > + if (dir == IIO_EV_DIR_RISING) > > + *val -= (*val2 * ret); > > + else > > + *val += (*val2 * ret); > > + > > + 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 * MCP9600_TEMP_SCALE_NUM) - val2) : > > + ((val * MCP9600_TEMP_SCALE_NUM) + val2); > > + > > + /* Hot junction temperature range is from –200 to 1800 degree celsius */ > > + if (chan->channel == MCP9600_CHAN_HOT_JUNCTION && > > + (s_val < (MCP9600_MIN_TEMP_HOT_JUNCTION * MCP9600_TEMP_SCALE_NUM) || > > + s_val > (MCP9600_MAX_TEMP_HOT_JUNCTION * MCP9600_TEMP_SCALE_NUM))) > > As above, change the units of the defines to simplify this or perhaps > just treat these as numbers and put them here rather than using defines at all. > Ok. > > + return -EINVAL; > > + > > + /* Cold junction temperature range is from –40 to 125 degree celsius */ > > + if (chan->channel == MCP9600_CHAN_COLD_JUNCTION && > > + (s_val < (MCP9600_MIN_TEMP_COLD_JUNCTION * MCP9600_TEMP_SCALE_NUM) || > > + s_val > (MCP9600_MAX_TEMP_COLD_JUNCTION * MCP9600_TEMP_SCALE_NUM))) > > + return -EINVAL; > > + > > + i = mcp9600_get_alert_index(chan->channel, dir); > > + guard(mutex)(&data->lock); > > + switch (info) { > > + case IIO_EV_INFO_VALUE: > > + /* > > + * Shift length 4 bits = 2(15:2) + 2(0.25 LSB), temperature is > > + * stored in two’s complement format. > > + */ > > + thresh = (s16)(s_val / (MCP9600_TEMP_SCALE_NUM >> 4)); > > + return i2c_smbus_write_word_swapped(client, > > + MCP9600_ALERT_LIMIT(i + 1), > > + thresh); > > + case IIO_EV_INFO_HYSTERESIS: > > + /* 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) * (MCP9600_TEMP_SCALE_NUM >> 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. > > + */ > > I don't understand this comment. > Hysteresis as a parameter in sysfs in IIO is also an offset, so why is the current > threshold relevant? > > Normally hysteresis is about allowing repeat events. I.e. you have to drop below > threshold - hysteresis before rising again to trigger a rising event when passing > threshold. > As above, I just didn't know better. > > > + hyst = min(255, abs(s_thresh - s_val) / MCP9600_TEMP_SCALE_NUM); > > + > > + return i2c_smbus_write_byte_data(client, > > + MCP9600_ALERT_HYSTERESIS(i + 1), > > + hyst); > > + default: > > + return -EINVAL; > > + } > > +} > > > > +static irqreturn_t mcp9600_alert3_handler(int irq, void *private) > > +{ > > + struct iio_dev *indio_dev = private; > > + struct mcp9600_data *data = iio_priv(indio_dev); > > + int ret; > > + > > + ret = i2c_smbus_read_byte_data(data->client, MCP9600_STATUS); > > + if (ret < 0) > > + return IRQ_HANDLED; > > + > > + if (!(ret & MCP9600_STATUS_ALERT(MCP9600_ALERT3))) > > This duplicates far too much all these call a function that takes > a) This bit, > b) the related channel index. > C) the direction > > and call that from all these separate handlers. > Each individual handler become simply: > > return mcp9600_alert_handler(private, MCP9600_ALERT3, > MCP9600_CHAN_COLD_JUNCTION, > IIO_EV_DIR_RISING); > > etc. > Yes, will use a helper function to avoid code duplication. > > + return IRQ_NONE; > > + > > + iio_push_event(indio_dev, > > + IIO_MOD_EVENT_CODE(IIO_TEMP, MCP9600_CHAN_COLD_JUNCTION, > > + IIO_NO_MOD, IIO_EV_TYPE_THRESH, > > + IIO_EV_DIR_RISING), > > + iio_get_time_ns(indio_dev)); > > + > > + return IRQ_HANDLED; > > +} > > + > > > + > > +static irqreturn_t (*mcp9600_alert_handler[MCP9600_ALERT_COUNT]) (int, void *) = { > > + mcp9600_alert1_handler, > > + mcp9600_alert2_handler, > > + mcp9600_alert3_handler, > > + mcp9600_alert4_handler, > > }; > Dimitri
