On Sat, Mar 12, 2016 at 12:17 PM, Jonathan Cameron <jic23@xxxxxxxxxx> wrote:
> On 09/03/16 13:30, Tiberiu Breana wrote:
>> Added interrupt support for high/low temperature threshold
>> events to the max31722 driver.
>>
>> Signed-off-by: Tiberiu Breana <tiberiu.a.breana@xxxxxxxxx>
> Only comment in here is that you might be better off reporting the
> event direction as IIO_EVENT_DIR_EITHER and leaving the figuring out
> which case occured to userspace.
>
> This case is iritatingly common!
>
> Jonathan
>> ---
>> drivers/iio/temperature/max31722.c | 303 ++++++++++++++++++++++++++++++++++++-
>> 1 file changed, 301 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/iio/temperature/max31722.c b/drivers/iio/temperature/max31722.c
>> index fa833b6..a8bfe35 100644
>> --- a/drivers/iio/temperature/max31722.c
>> +++ b/drivers/iio/temperature/max31722.c
>> @@ -10,21 +10,32 @@
>>
>> #include <linux/kernel.h>
>> #include <linux/acpi.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/interrupt.h>
>> #include <linux/module.h>
>> #include <linux/regmap.h>
>> #include <linux/spi/spi.h>
>> +#include <linux/iio/events.h>
>> #include <linux/iio/iio.h>
>> #include <linux/iio/sysfs.h>
>>
>> #define MAX31722_REG_CFG 0x00
>> #define MAX31722_REG_TEMP_LSB 0x01
>> #define MAX31722_REG_TEMP_MSB 0x02
>> +#define MAX31722_REG_THIGH_LSB 0x03
>> +#define MAX31722_REG_TLOW_LSB 0x05
>> #define MAX31722_MAX_REG 0x86
>>
>> #define MAX31722_MODE_CONTINUOUS 0x00
>> #define MAX31722_MODE_STANDBY 0x01
>> +#define MAX31722_MODE_THERMOSTAT 0x80
>> +
>> +#define MAX31722_MIN_TEMP -55
>> +#define MAX31722_MAX_TEMP 125
>>
>> #define MAX31722_REGMAP_NAME "max31722_regmap"
>> +#define MAX31722_EVENT "max31722_event"
>> +#define MAX31722_GPIO "max31722_gpio"
>>
>> #define MAX31722_SCALE_AVAILABLE "0.5 0.25 0.125 0.0625"
>>
>> @@ -43,6 +54,8 @@ static const struct reg_field max31722_reg_field_state =
>> REG_FIELD(MAX31722_REG_CFG, 0, 0);
>> static const struct reg_field max31722_reg_field_resolution =
>> REG_FIELD(MAX31722_REG_CFG, 1, 2);
>> +static const struct reg_field max31722_reg_field_thermostat =
>> + REG_FIELD(MAX31722_REG_CFG, 3, 3);
>>
>> struct max31722_data {
>> struct spi_device *spi_device;
>> @@ -50,6 +63,8 @@ struct max31722_data {
>> struct regmap *regmap;
>> struct regmap_field *reg_state;
>> struct regmap_field *reg_resolution;
>> + struct regmap_field *reg_thermostat;
>> + u64 timestamp;
>> };
>>
>> /*
>> @@ -71,11 +86,30 @@ static const struct attribute_group max31722_attribute_group = {
>> .attrs = max31722_attributes
>> };
>>
>> +static const struct iio_event_spec max31722_events[] = {
>> + /* High temperature event */
>> + {
>> + .type = IIO_EV_TYPE_THRESH,
>> + .dir = IIO_EV_DIR_RISING,
>> + .mask_separate = BIT(IIO_EV_INFO_VALUE) |
>> + BIT(IIO_EV_INFO_ENABLE),
>> + },
>> + /* Low temperature event */
>> + {
>> + .type = IIO_EV_TYPE_THRESH,
>> + .dir = IIO_EV_DIR_FALLING,
>> + .mask_separate = BIT(IIO_EV_INFO_VALUE) |
>> + BIT(IIO_EV_INFO_ENABLE),
>> + },
>> +};
>> +
>> static const struct iio_chan_spec max31722_channels[] = {
>> {
>> .type = IIO_TEMP,
>> .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
>> BIT(IIO_CHAN_INFO_SCALE),
>> + .event_spec = max31722_events,
>> + .num_event_specs = ARRAY_SIZE(max31722_events),
>> },
>> };
>>
>> @@ -167,6 +201,172 @@ static void max31722_reg_to_float(u16 reg_val, int *val, int *val2)
>> }
>> }
>>
>> +/* Convert a floating point value to a register value. */
>> +static u16 max31722_float_to_reg(int val, int val2)
>> +{
>> + int i;
>> + bool negative_nr;
>> + u8 lsb;
>> + u16 reg_val;
>> +
>> + negative_nr = (val & 0x80) || val2 < 0;
>> + if (val2 < 0)
>> + val2 *= -1;
>> +
>> + /*
>> + * The LSB value will be an approximation of val2
>> + * due to its limited 4-bit range.
>> + */
>> + lsb = 0;
>> + for (i = 0 ; i < ARRAY_SIZE(max31722_scale_table) && val2 > 0; i++)
>> + if (val2 - max31722_scale_table[i] >= 0) {
>> + val2 -= max31722_scale_table[i];
>> + lsb += 1 << (4 - i - 1);
>> + }
>> + lsb <<= 4;
>> +
>> + if (negative_nr) {
>> + /*
>> + * Negative value. Temporarily use the complement of val for
>> + * the MSB, then concatenate the LSB, reverse bits & add 1 for
>> + * the final value.
>> + */
>> + u8 msb = (u8)(-1 * val);
>> + u16 rev = 0;
>> + int num_bits = sizeof(rev) * 8;
>> +
>> + reg_val = (msb << 8) | lsb;
>> +
>> + for (i = 0 ; i < num_bits ; i++) {
>> + rev |= !(reg_val & 0x01) << i;
>> + reg_val >>= 1;
>> + }
>> + rev += 1;
>> + return rev;
>> +
>> + } else {
>> + reg_val = ((u8)val << 8) | lsb;
>> + }
>> +
>> + return reg_val;
>> +}
>> +
>> +static int max31722_read_event_config(struct iio_dev *indio_dev,
>> + const struct iio_chan_spec *chan,
>> + enum iio_event_type type,
>> + enum iio_event_direction dir)
>> +{
>> + unsigned int event_val;
>> + int ret;
>> + struct max31722_data *data = iio_priv(indio_dev);
>> + struct spi_device *spi = data->spi_device;
>> +
>> + mutex_lock(&data->lock);
>> + ret = regmap_field_read(data->reg_thermostat, &event_val);
>> + if (ret < 0) {
>> + dev_err(&spi->dev, "failed to read thermostat status\n");
>> + mutex_unlock(&data->lock);
>> + return ret;
>> + }
>> + mutex_unlock(&data->lock);
>> +
>> + return event_val;
>> +}
>> +
>> +static int max31722_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)
>> +{
>> + int ret;
>> + struct max31722_data *data = iio_priv(indio_dev);
>> + struct spi_device *spi = data->spi_device;
>> +
>> + if (state != 0 && state != 1)
>> + return -EINVAL;
>> +
>> + /*
>> + * Set thermostat mode:
>> + * 0 : comparator mode (default)
>> + * 1 : interrupt mode
>> + */
>> + mutex_lock(&data->lock);
>> + ret = regmap_field_write(data->reg_thermostat, state);
>> + if (ret < 0)
>> + dev_err(&spi->dev, "failed to set thermostat mode\n");
>> + mutex_unlock(&data->lock);
>> +
>> + return ret;
>> +}
>> +
>> +static int max31722_read_event(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)
>> +{
>> + int ret;
>> + u8 reg;
>> + u16 buf;
>> + struct max31722_data *data = iio_priv(indio_dev);
>> +
>> + if (info != IIO_EV_INFO_VALUE)
>> + return -EINVAL;
>> +
>> + if (dir == IIO_EV_DIR_RISING)
>> + reg = MAX31722_REG_THIGH_LSB;
>> + else if (dir == IIO_EV_DIR_FALLING)
>> + reg = MAX31722_REG_TLOW_LSB;
>> + else
>> + return -EINVAL;
>> +
>> + mutex_lock(&data->lock);
>> + ret = regmap_bulk_read(data->regmap, reg, &buf, 2);
>> + mutex_unlock(&data->lock);
>> + if (ret < 0) {
>> + dev_err(&data->spi_device->dev,
>> + "failed to read threshold register\n");
>> + return ret;
>> + }
>> + max31722_reg_to_float(buf, val, val2);
>> +
>> + return IIO_VAL_INT_PLUS_MICRO;
>> +}
>> +
>> +static int max31722_write_event(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)
>> +{
>> + int ret;
>> + u8 reg;
>> + u16 buf;
>> + struct max31722_data *data = iio_priv(indio_dev);
>> +
>> + if (val < MAX31722_MIN_TEMP || val > MAX31722_MAX_TEMP)
>> + return -EINVAL;
>> +
>> + if (dir == IIO_EV_DIR_RISING)
>> + reg = MAX31722_REG_THIGH_LSB;
>> + else if (dir == IIO_EV_DIR_FALLING)
>> + reg = MAX31722_REG_TLOW_LSB;
>> + else
>> + return -EINVAL;
>> +
>> + buf = max31722_float_to_reg(val, val2);
>> +
>> + ret = regmap_bulk_write(data->regmap, reg, &buf, 2);
>> + if (ret < 0)
>> + dev_err(&data->spi_device->dev,
>> + "failed to write threshold register\n");
>> +
>> + return ret;
>> +}
>> +
>> static int max31722_read_raw(struct iio_dev *indio_dev,
>> struct iio_chan_spec const *chan,
>> int *val, int *val2, long mask)
>> @@ -241,8 +441,63 @@ static const struct iio_info max31722_info = {
>> .read_raw = max31722_read_raw,
>> .write_raw = max31722_write_raw,
>> .attrs = &max31722_attribute_group,
>> + .read_event_value = max31722_read_event,
>> + .write_event_value = max31722_write_event,
>> + .read_event_config = max31722_read_event_config,
>> + .write_event_config = max31722_write_event_config,
>> };
>>
>> +static irqreturn_t max31722_irq_handler(int irq, void *private)
>> +{
>> + struct iio_dev *indio_dev = private;
>> + struct max31722_data *data = iio_priv(indio_dev);
>> +
>> + data->timestamp = iio_get_time_ns();
>> +
>> + return IRQ_WAKE_THREAD;
>> +}
>> +
>> +static irqreturn_t max31722_irq_event_handler(int irq, void *private)
>> +{
>> + int ret;
>> + u64 event;
>> + u16 temp;
>> + u16 tlow;
>> + u16 thigh;
>> + unsigned int dir;
>> + struct iio_dev *indio_dev = private;
>> + struct max31722_data *data = iio_priv(indio_dev);
>> +
>> + /*
>> + * Do a quick temperature reading and compare it with TLOW/THIGH
>> + * so we can tell which threshold has been met.
> Hmm. Might end up detecting neither occured if the condition is no longer
> met. Iritating hardware!
>
> We do have the 'magic' option of IIO_EV_DIR_EITHER to specify that we
> don't know which one occured. The idea of that has always been that
> userspace can elect to do what you have here if it cares rather than
> pushing this code into drivers.
>> + */
>> + mutex_lock(&data->lock);
>> + ret = regmap_bulk_read(data->regmap, MAX31722_REG_TEMP_LSB, &temp, 2);
>> + if (ret < 0)
>> + goto exit_err;
>> + ret = regmap_bulk_read(data->regmap, MAX31722_REG_TLOW_LSB, &tlow, 2);
>> + if (ret < 0)
>> + goto exit_err;
>> + ret = regmap_bulk_read(data->regmap, MAX31722_REG_THIGH_LSB, &thigh, 2);
>> + if (ret < 0)
>> + goto exit_err;
>> + mutex_unlock(&data->lock);
>> +
>> + dir = (temp > thigh ? 1 : 0);
>> + event = IIO_UNMOD_EVENT_CODE(IIO_TEMP, 0, IIO_EV_TYPE_THRESH,
>> + (dir ? IIO_EV_DIR_RISING :
>> + IIO_EV_DIR_FALLING));
>> + iio_push_event(indio_dev, event, data->timestamp);
>> +
>> + return IRQ_HANDLED;
>> +
>> +exit_err:
>> + dev_err(&data->spi_device->dev, "failed to read temperature register\n");
>> + mutex_unlock(&data->lock);
>> + return ret;
>> +}
>> +
>> static int max31722_init(struct max31722_data *data)
>> {
>> int ret = 0;
>> @@ -259,15 +514,40 @@ static int max31722_init(struct max31722_data *data)
>>
>> MAX31722_REGFIELD(state);
>> MAX31722_REGFIELD(resolution);
>> + MAX31722_REGFIELD(thermostat);
>>
>> /* Set SD bit to 0 so we can have continuous measurements. */
>> ret = regmap_field_write(data->reg_state, MAX31722_MODE_CONTINUOUS);
>> + if (ret < 0) {
>> + dev_err(&spi->dev, "failed to configure sensor.\n");
>> + return ret;
>> + }
>> +
>> + /* Set thermostat interrupt mode */
>> + ret = regmap_field_write(data->reg_thermostat, 1);
>> if (ret < 0)
>> dev_err(&spi->dev, "failed to configure sensor.\n");
>>
>> return ret;
>> }
>>
>> +static int max31722_gpio_probe(struct spi_device *spi)
>> +{
>> + struct gpio_desc *gpio;
>> +
>> + if (!spi)
>> + return -EINVAL;
>> +
>> + /* gpio interrupt pin */
>> + gpio = devm_gpiod_get_index(&spi->dev, MAX31722_GPIO, 0, GPIOD_IN);
>> + if (IS_ERR(gpio)) {
>> + dev_err(&spi->dev, "gpio request failed.\n");
>> + return PTR_ERR(gpio);
>> + }
> If this is an interrupt, why are we specifying it as a gpio?
It is an interrupt requested on a GPIO pin. Isn't this the standard
way of mapping
a GPIO pin to an IRQ? Anyhow, as explained below this is no longer needed.
>> +
>> + return gpiod_to_irq(gpio);
>> +}
>> +
>> static int max31722_probe(struct spi_device *spi)
>> {
>> int ret;
>> @@ -294,15 +574,34 @@ static int max31722_probe(struct spi_device *spi)
>>
>> ret = max31722_init(data);
>> if (ret < 0)
>> - return ret;
>> + goto err_standby;
>> +
>> + ret = max31722_gpio_probe(data->spi_device);
>> + if (ret < 0)
>> + goto err_standby;
>> +
>> + spi->irq = ret;
If ACPI or DT is properly configured spi->irq should already contain the correct
irq number. We used the gpio_probe approach when ACPI didn't save the interrupt
number. Newer kernel version shouldn't have this problem.
>> + ret = devm_request_threaded_irq(&spi->dev, spi->irq,
>> + max31722_irq_handler,
>> + max31722_irq_event_handler,
>> + IRQF_TRIGGER_LOW,
>> + MAX31722_EVENT, indio_dev);
>> + if (ret < 0) {
>> + dev_err(&spi->dev, "request irq %d failed\n", spi->irq);
>> + goto err_standby;
>> + }
> I would suggest you want to make the irq optional, though that could happen
> at a later date when someone has a board where it isn't wired.
Agree. We had previous experience with unwired interrupts pins.
>>
>> ret = iio_device_register(indio_dev);
>> if (ret < 0) {
>> dev_err(&spi->dev, "iio_device_register failed\n");
>> - regmap_field_write(data->reg_state, MAX31722_MODE_STANDBY);
>> + goto err_standby;
>> }
>>
>> return ret;
>> +
>> +err_standby:
>> + regmap_field_write(data->reg_state, MAX31722_MODE_STANDBY);
>> + return ret;
>> }
>>
>> static int max31722_remove(struct spi_device *spi)
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
