Re: [PATCH v5] iio: temperature: mcp9600: add threshold events support

From: Jonathan Cameron
Date: Sun Jun 09 2024 - 05:05:03 EST

On Tue, 4 Jun 2024 15:36:39 +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>

I think I'd have done this by taking a copy of the channels array and
putting just replacing the .event_spec and .num_event_specs fields
but what you have here works so fair enough. It's always a fine
balance between when it makes sense to make everything data that you
pick from and when you use code to tweak that data a little.

Applied to the togreg branch of iio.git and pushed out as testing
for 0-day to take a look.

Thanks,

Jonathan

> ---
>
> Changes in V2:
> - Remove pretty printing patches from series
> - Add patch for providing index for both channels(ABI change)!
> Suggested by Jonathan, hope this okay.
> - Remove formatting in a precursor patch
> - Add lock documentation
> - Add define MCP9600_TEMP_SCALE_NUM and use it instead of MICRO. MICRO is
> type unsigned long which we have to cast to int when using
> multiplication or division, because we are handling negative values.
> - Use switch statement in mcp9600_write_thresh
> - Replaced generic interrupt handler with four separate interrupt handler
> - Use one lock instead of four
> - Added error check for mcp9600_probe_alerts
>
> Changes in V3:
> - Remove patch for providing index for both channels(ABI change)!
> - Treat hysteresis as offset and remove the lock, since dependency
> between hysteresis and threshold doesn't exist anymore.
> - Use helper function for handling alerts to avoid code duplication
> - Scale max,min defines for hot and cold junction temperature to micro
>
> Changes in V4:
> - Added version number of patch
>
> Changes in V5:
> - move "struct iio_dev *indio_dev = private;" to mcp9600_alert_handler
> instead of setting it in each alert handler to avoid code duplication
> - Only create sysfs interfaces for events which are present.
>
> ---
> drivers/iio/temperature/mcp9600.c | 363 ++++++++++++++++++++++++++++--
> 1 file changed, 349 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/iio/temperature/mcp9600.c b/drivers/iio/temperature/mcp9600.c
> index 46845804292b..bfa873731d61 100644
> --- a/drivers/iio/temperature/mcp9600.c
> +++ b/drivers/iio/temperature/mcp9600.c
> @@ -6,39 +6,123 @@
> * 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/iio/events.h>
> #include <linux/iio/iio.h>
>
> /* 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
>
> -static const struct iio_chan_spec mcp9600_channels[] = {
> +#define MCP9600_ALERT_COUNT 4
> +
> +#define MCP9600_MIN_TEMP_HOT_JUNCTION_MICRO -200000000
> +#define MCP9600_MAX_TEMP_HOT_JUNCTION_MICRO 1800000000
> +
> +#define MCP9600_MIN_TEMP_COLD_JUNCTION_MICRO -40000000
> +#define MCP9600_MAX_TEMP_COLD_JUNCTION_MICRO 125000000
> +
> +enum mcp9600_alert {
> + MCP9600_ALERT1,
> + MCP9600_ALERT2,
> + MCP9600_ALERT3,
> + MCP9600_ALERT4
> +};
> +
> +static const char * const mcp9600_alert_name[MCP9600_ALERT_COUNT] = {
> + [MCP9600_ALERT1] = "alert1",
> + [MCP9600_ALERT2] = "alert2",
> + [MCP9600_ALERT3] = "alert3",
> + [MCP9600_ALERT4] = "alert4",
> +};
> +
> +static const struct iio_event_spec mcp9600_events[] = {
> {
> - .type = IIO_TEMP,
> - .address = MCP9600_HOT_JUNCTION,
> - .info_mask_separate =
> - BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_RISING,
> + .mask_separate = BIT(IIO_EV_INFO_ENABLE) |
> + BIT(IIO_EV_INFO_VALUE) |
> + BIT(IIO_EV_INFO_HYSTERESIS),
> },
> {
> - .type = IIO_TEMP,
> - .address = MCP9600_COLD_JUNCTION,
> - .channel2 = IIO_MOD_TEMP_AMBIENT,
> - .modified = 1,
> - .info_mask_separate =
> - BIT(IIO_CHAN_INFO_RAW) | BIT(IIO_CHAN_INFO_SCALE),
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_FALLING,
> + .mask_separate = BIT(IIO_EV_INFO_ENABLE) |
> + BIT(IIO_EV_INFO_VALUE) |
> + BIT(IIO_EV_INFO_HYSTERESIS),
> },
> };
>
> +#define MCP9600_CHANNELS(hj_num_ev, hj_ev_spec_off, cj_num_ev, cj_ev_spec_off) \
> + { \
> + { \
> + .type = IIO_TEMP, \
> + .address = MCP9600_HOT_JUNCTION, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_SCALE), \
> + .event_spec = &mcp9600_events[hj_ev_spec_off], \
> + .num_event_specs = hj_num_ev, \
> + }, \
> + { \
> + .type = IIO_TEMP, \
> + .address = MCP9600_COLD_JUNCTION, \
> + .channel2 = IIO_MOD_TEMP_AMBIENT, \
> + .modified = 1, \
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) | \
> + BIT(IIO_CHAN_INFO_SCALE), \
> + .event_spec = &mcp9600_events[cj_ev_spec_off], \
> + .num_event_specs = cj_num_ev, \
> + }, \
> + }
> +
> +static const struct iio_chan_spec mcp9600_channels[][2] = {
> + MCP9600_CHANNELS(0, 0, 0, 0), /* Alerts: - - - - */
> + MCP9600_CHANNELS(1, 0, 0, 0), /* Alerts: 1 - - - */
> + MCP9600_CHANNELS(1, 1, 0, 0), /* Alerts: - 2 - - */
> + MCP9600_CHANNELS(2, 0, 0, 0), /* Alerts: 1 2 - - */
> + MCP9600_CHANNELS(0, 0, 1, 0), /* Alerts: - - 3 - */
> + MCP9600_CHANNELS(1, 0, 1, 0), /* Alerts: 1 - 3 - */
> + MCP9600_CHANNELS(1, 1, 1, 0), /* Alerts: - 2 3 - */
> + MCP9600_CHANNELS(2, 0, 1, 0), /* Alerts: 1 2 3 - */
> + MCP9600_CHANNELS(0, 0, 1, 1), /* Alerts: - - - 4 */
> + MCP9600_CHANNELS(1, 0, 1, 1), /* Alerts: 1 - - 4 */
> + MCP9600_CHANNELS(1, 1, 1, 1), /* Alerts: - 2 - 4 */
> + MCP9600_CHANNELS(2, 0, 1, 1), /* Alerts: 1 2 - 4 */
> + MCP9600_CHANNELS(0, 0, 2, 0), /* Alerts: - - 3 4 */
> + MCP9600_CHANNELS(1, 0, 2, 0), /* Alerts: 1 - 3 4 */
> + MCP9600_CHANNELS(1, 1, 2, 0), /* Alerts: - 2 3 4 */
> + MCP9600_CHANNELS(2, 0, 2, 0), /* Alerts: 1 2 3 4 */
> +};