[2/2] iio: temperature: Add support for AMS AS6200
Commit Message
as6200 is a high accuracy temperature sensor of 0.0625C with a range
between -40 to 125 Celsius degrees. The driver implements the alert
trigger event in comparator mode where an alert would trigger
when n number of consecutive current temperature is above the upper
temp limit, and the alert is only cleared when the n number of
consecutive current temperature is below the lower temp limit.
The driver supports the following:
- show available sampling frequencey
- read/write sampling frequency
- read raw temperature
- read scaling factor
- read/write consective faults to trigger/clear an alert
- show available consecutive faults
- buffer trigger
- temperature alert event trigger
https://ams.com/documents/20143/36005/AS6200_DS000449_4-00.pdf
Signed-off-by: Abdel Alkuor <alkuor@gmail.com>
---
drivers/iio/temperature/Kconfig | 9 +
drivers/iio/temperature/Makefile | 1 +
drivers/iio/temperature/as6200.c | 507 +++++++++++++++++++++++++++++++
3 files changed, 517 insertions(+)
create mode 100644 drivers/iio/temperature/as6200.c
Comments
On Sat, 11 Nov 2023 17:05:02 -0500
Abdel Alkuor <alkuor@gmail.com> wrote:
> as6200 is a high accuracy temperature sensor of 0.0625C with a range
> between -40 to 125 Celsius degrees. The driver implements the alert
> trigger event in comparator mode where an alert would trigger
> when n number of consecutive current temperature is above the upper
> temp limit, and the alert is only cleared when the n number of
> consecutive current temperature is below the lower temp limit.
>
Hi Abdel,
Comments inline. Sorry it took so long for you get a review. I've been
travelling + snowed under since returning.
Seems like a very feature rich driver whilst still being nice and short :)
> The driver supports the following:
> - show available sampling frequencey
> - read/write sampling frequency
> - read raw temperature
> - read scaling factor
> - read/write consective faults to trigger/clear an alert
> - show available consecutive faults
> - buffer trigger
> - temperature alert event trigger
>
> https://ams.com/documents/20143/36005/AS6200_DS000449_4-00.pdf
Use a formal tag in the tag block for this.
>
Datasheet: https://ams.com/documents/20143/36005/AS6200_DS000449_4-00.pdf
> Signed-off-by: Abdel Alkuor <alkuor@gmail.com>
> ---
> drivers/iio/temperature/Kconfig | 9 +
> drivers/iio/temperature/Makefile | 1 +
> drivers/iio/temperature/as6200.c | 507 +++++++++++++++++++++++++++++++
> 3 files changed, 517 insertions(+)
> create mode 100644 drivers/iio/temperature/as6200.c
>
> diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
> index ed384f33e0c7..f32691744952 100644
> --- a/drivers/iio/temperature/Kconfig
> +++ b/drivers/iio/temperature/Kconfig
> @@ -157,5 +157,14 @@ config MAX31865
>
> This driver can also be build as a module. If so, the module
> will be called max31865.
blank line. Also should be in alphabetical order, not down the end of the file.
Appending to the end causes a lot more trouble for merging multiple series than
alphabetical order, which is one reason we always try to keep these files in order.
> +config AS6200
> + tristate "AS6200 temperature sensor"
> + depends on I2C
> + help
> + If you say yes here you get support for AS6200
> + temperature sensor chip connected via I2C.
> +
> + This driver can also be built as a module. If so, the module
> + will be called as6200.
>
> endmenu
> diff --git a/drivers/iio/temperature/Makefile b/drivers/iio/temperature/Makefile
> index dfec8c6d3019..48f647c273c1 100644
> --- a/drivers/iio/temperature/Makefile
> +++ b/drivers/iio/temperature/Makefile
> @@ -17,3 +17,4 @@ obj-$(CONFIG_TMP007) += tmp007.o
> obj-$(CONFIG_TMP117) += tmp117.o
> obj-$(CONFIG_TSYS01) += tsys01.o
> obj-$(CONFIG_TSYS02D) += tsys02d.o
> +obj-$(CONFIG_AS6200) += as6200.o
Alphabetical order.
> diff --git a/drivers/iio/temperature/as6200.c b/drivers/iio/temperature/as6200.c
> new file mode 100644
> index 000000000000..a18c5be0a229
> --- /dev/null
> +++ b/drivers/iio/temperature/as6200.c
> @@ -0,0 +1,507 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Driver for AMS AS6200 Temperature sensor
> + *
> + * Auther: Abdel Alkuor <alkuor@gmail.com>
Spell check comments Author.
> + */
> +
> +#include <linux/i2c.h>
Alphabetical order, but it's common to have the iio/*
headers as a separate block afterwards given this is an IIO driver.
> +#include <linux/module.h>
> +#include <linux/regmap.h>
> +#include <linux/interrupt.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/events.h>
> +#include <linux/iio/trigger.h>
> +#include <linux/iio/buffer.h>
> +#include <linux/iio/triggered_buffer.h>
> +#include <linux/iio/trigger_consumer.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/device.h>
> +#include <linux/bitfield.h>
> +#include <linux/kstrtox.h>
> +
> +#define AS_TVAL_REG 0x0
Probably want a more specific prefix.
AS6200_TVAL_REG etc
> +#define AS_CONFIG_REG 0x1
> +#define AS_TLOW_REG 0x2
> +#define AS_THIGH_REG 0x3
> +
> +/* AS_CONFIG_REG */
The register and field naming should make it clear enough which register
these are in. It think that is true here so you don't need the comment.
Excess comments are just opportunities for code rot so keep them for when
they add significant value.
> +#define AS_CONFIG_AL BIT(5)
> +#define AS_CONFIG_CR GENMASK(7, 6)
> +#define AS_CONFIG_SM BIT(8)
> +#define AS_CONFIG_IM BIT(9)
> +#define AS_CONFIG_POL BIT(10)
> +#define AS_CONFIG_CF GENMASK(12, 11)
> +
> +#define AS_TEMP_SCALE 62500
This isn't a magic number, it's an actual quantity. As such, just put the
number inline in the code rather than use a define that just makes it harder
to review.
> +
> +struct as6200_freq {
> + int val;
> + int val2;
> +};
I don't mind the structure, but not sure it adds much over a simple 2d array of
integers given the ordering of val and val2 is fairly natural anyway.
> +
> +struct as6200 {
> + struct device *dev;
> + struct regmap *regmap;
> + struct mutex lock;
All locks need a comment to explain what data they are protecting.
> + struct iio_dev *indio_dev;
You may have noticed this is almost never done in IIO drivers. It normally
indicates a problem with the architecture. There is a natural way to go
from iio_dev to iio_priv() so we shouldn't nee to go the other way.
If you did need to have a remove() (which you don't - see later) then
should by the struct iio_dev * in the i2c_clientdata not the
iio_priv() structure.
> +
> + unsigned int data[3];
> +};
> +
> +static const struct as6200_freq as6200_samp_freq[4] = {
> + {0, 250000},
Trivial: Slight preference for more spaces { 0, 2500000 },
> + {1, 0},
> + {4, 0},
> + {8, 0},
> +};
> +
> +static const struct iio_event_spec as6200_temp_event[] = {
> + {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_RISING,
> + .mask_separate = BIT(IIO_EV_INFO_VALUE)
> + },
> + {
> + .type = IIO_EV_TYPE_THRESH,
> + .dir = IIO_EV_DIR_FALLING,
> + .mask_separate = BIT(IIO_EV_INFO_VALUE)
> + },
> +};
> +
> +static const struct iio_chan_spec as6200_temp_channels[] = {
There aren't any other channels so as6200_channels is enough.
> + {
> + .type = IIO_TEMP,
> + .indexed = 0,
That's the 'obvious' default so don't provide it.
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_SCALE) |
> + BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + .scan_index = 0,
With .indexed =0, this isn't used so no need to specify it here.
> + .scan_type = {
> + .sign = 's',
> + .realbits = 12,
> + .storagebits = 16,
> + .shift = 4,
> + },
> + .event_spec = as6200_temp_event,
> + .num_event_specs = ARRAY_SIZE(as6200_temp_event),
> + },
> + IIO_CHAN_SOFT_TIMESTAMP(1),
> +};
> +
> +static const struct regmap_config as6200_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 16,
> + .max_register = 0x7F,
> +};
> +
> +static int
> +as6200_modify_config_reg(struct as6200 *as, unsigned int mask, unsigned int val)
> +{
> + int ret;
> + unsigned int reg;
> +
> + ret = regmap_read(as->regmap, AS_CONFIG_REG, ®);
> + if (ret)
> + return ret;
> +
> + reg &= ~mask;
> + reg |= val << (ffs(mask) - 1);
> +
> + return regmap_write(as->regmap, AS_CONFIG_REG, reg);
regmap has functions to handle a read modify write cycle as simple as this.
Use those instead of inventing your own. regmap_update_bits() for example
They also hold the regmap lock over the whole cycle which can simplify locking
requirements on a driver using it.
> +}
> +
> +static int
> +as6200_read_config_reg(struct as6200 *as, unsigned int mask, unsigned int *val)
All the masks passed to this are constant, so it would be better to just
put something like
ret = regmap_read(as->regmap, AS_CONFIG_REG, ®);
if (ret)
return ret;
x = FIELD_GET(reg, MASK);
inline and get rid of this function entirely.
> +{
> + int ret;
> + unsigned int reg;
> +
> + ret = regmap_read(as->regmap, AS_CONFIG_REG, ®);
> + if (ret)
> + return ret;
> +
> + *val = (reg & mask) >> (ffs(mask) - 1);
> +
> + return 0;
> +}
> +
> +
> +static int as6200_read_event_value(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 as6200 *as = iio_device_get_drvdata(indio_dev);
> + unsigned int reg;
> + int ret;
> + unsigned int temp;
> +
> + switch (dir) {
> + case IIO_EV_DIR_FALLING:
> + reg = AS_TLOW_REG;
> + break;
> + case IIO_EV_DIR_RISING:
> + reg = AS_THIGH_REG;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + ret = regmap_read(as->regmap, reg, &temp);
> + if (ret)
> + return ret;
> +
> + *val = sign_extend32(temp >> 4, 11);
That shift looks like it should be handled via a FIELD_GET()
an appropriate mask.
> +
> + return IIO_VAL_INT;
> +}
> +
> +static int as6200_write_event_value(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 as6200 *as = iio_device_get_drvdata(indio_dev);
Why? You have the iio_dev so
struct as6000 *as = iio_priv(indio_dev);
> + unsigned int temp;
> + unsigned int reg;
> +
> + /*
> + * range without applying the scaling
> + * factor of 0.06250
> + */
> + if (val > 2000 || val < -640)
> + return -EINVAL;
> +
> + temp = (val & 0xfff) << 4;
FIELD_PREP() with appropriately defined _MASK
> +
> + switch (dir) {
> + case IIO_EV_DIR_FALLING:
> + reg = AS_TLOW_REG;
> + break;
> + case IIO_EV_DIR_RISING:
> + reg = AS_THIGH_REG;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + return regmap_write(as->regmap, reg, temp);
> +}
> +
> +static irqreturn_t as6200_event_handler(int irq, void *private)
> +{
> + struct iio_dev *indio_dev = private;
> + struct as6200 *as = iio_device_get_drvdata(indio_dev);
As above.
> + unsigned int alert;
> + enum iio_event_direction dir;
> + int ret;
> +
> + mutex_lock(&as->lock);
guard(mutex)(&as->lock);
then you don't need to do any manual unlocking.
> +
> + ret = as6200_read_config_reg(as, AS_CONFIG_AL, &alert);
> + if (ret) {
> + mutex_unlock(&as->lock);
> + return IRQ_NONE;
> + }
> +
> + dir = alert ? IIO_EV_DIR_FALLING : IIO_EV_DIR_RISING;
> +
> + iio_push_event(indio_dev,
> + IIO_EVENT_CODE(IIO_TEMP, 0, 0,
> + dir,
> + IIO_EV_TYPE_THRESH,
> + 0, 0, 0),
> + iio_get_time_ns(indio_dev));
> +
> + mutex_unlock(&as->lock);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t as6200_trigger_handler(int irq, void *private)
> +{
> + struct iio_poll_func *pf = private;
> + struct iio_dev *indio_dev = pf->indio_dev;
> + struct as6200 *as = iio_device_get_drvdata(indio_dev);
> + int ret;
> +
> + mutex_lock(&as->lock);
scoped_guard(mutex, &as->lock) {
ret = regmap_read(as->regmap, AS_TVAL_REG, &as->data[0]);
if (ret)
break;
iio_push_to_buffers_with_timestamp(indio_dev, as->data,
iio_get_time_ns(indio_dev));
However, data isn't big enough. It should contain enough space for the data +
a naturally aligned s64 (so 16 bytes). This interface is deeply unintuitive
but we have been stuck with it for a long time now :(
Also, I'm not sure why we can't just have data on the stack and if we do,
is there a need for the lock?
}
iio_trigger_notify_done()....
> +
> + ret = regmap_read(as->regmap, AS_TVAL_REG, &as->data[0]);
> + if (!ret)
> + iio_push_to_buffers_with_timestamp(indio_dev, as->data,
> + iio_get_time_ns(indio_dev));
> +
> + mutex_unlock(&as->lock);
> +
> + iio_trigger_notify_done(indio_dev->trig);
> +
> + return IRQ_HANDLED;
> +}
> +
> +
> +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("0.25 1 4 8");
> +static IIO_CONST_ATTR(avail_consecutive_faults, "1 2 4 6");
> +static IIO_DEVICE_ATTR_RW(consecutive_faults, 0);
> +
> +static struct attribute *as6200_event_attributes[] = {
> + &iio_const_attr_avail_consecutive_faults.dev_attr.attr,
> + &iio_dev_attr_consecutive_faults.dev_attr.attr,
Custom ABI, so should be some documentation.
I'm guessing a bit, but sounds like the standard ABI period
to me.
https://elixir.bootlin.com/linux/latest/source/Documentation/ABI/testing/sysfs-bus-iio#L1163
However you will need to take into account current sampling frequency
and provide this in seconds as per the documentation.
It's absolutely fine to have available change with the sampling
frequency.
> + NULL,
> +};
> +
> +static struct attribute *as6200_attributes[] = {
> + &iio_const_attr_sampling_frequency_available.dev_attr.attr,
For this one, use read_avail() and appropriate bitmaps.
That both reduces manual handling of attributes and allows in kernel
consumers to potentially access this parameter.
> + NULL,
> +};
> +
> +static const struct attribute_group as6200_attribute_group = {
> + .attrs = as6200_attributes,
> +};
> +
> +static const struct attribute_group as6200_event_attribute_group = {
> + .attrs = as6200_event_attributes,
> +};
> +
> +static const struct iio_info as6200_temp_info = {
> + .event_attrs = &as6200_event_attribute_group,
> + .attrs = &as6200_attribute_group,
> + .read_raw = &as6200_read_raw,
> + .write_raw = &as6200_write_raw,
> + .read_event_value = &as6200_read_event_value,
> + .write_event_value = &as6200_write_event_value,
> +};
> +
> +static int as6200_probe(struct i2c_client *client)
> +{
> + struct as6200 *as;
> + struct iio_dev *indio_dev;
> + int ret;
> +
> + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> + return -EINVAL;
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, 0);
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + as = devm_kzalloc(&client->dev, sizeof(*as), GFP_KERNEL);
> + if (IS_ERR(as))
> + return PTR_ERR(as);
> +
> + as->regmap = devm_regmap_init_i2c(client, &as6200_regmap_config);
> + if (IS_ERR(as->regmap))
> + return PTR_ERR(as->regmap);
> +
> + mutex_init(&as->lock);
> +
> + as->dev = &client->dev;
> + as->indio_dev = indio_dev;
> +
> + iio_device_set_drvdata(indio_dev, as);
> +
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->channels = as6200_temp_channels;
> + indio_dev->num_channels = ARRAY_SIZE(as6200_temp_channels);
> + indio_dev->name = client->name;
Encode the string directly here. The different firmware types fill client->name
in via various fragile means. Things are safer if we don't rely on that.
> + indio_dev->info = &as6200_temp_info;
> +
> + ret = devm_iio_triggered_buffer_setup(as->dev, indio_dev,
> + NULL,
> + as6200_trigger_handler,
> + NULL);
> + if (ret)
> + return ret;
> +
> + if (client->irq) {
> + ret = devm_request_threaded_irq(as->dev,
> + client->irq,
> + NULL,
> + &as6200_event_handler,
> + IRQF_ONESHOT,
> + client->name,
> + indio_dev);
> + if (ret)
> + return ret;
> + }
> +
> + i2c_set_clientdata(client, as);
> +
> + return iio_device_register(indio_dev);
As only thing in remove is iio_device_unregister() use
return devm_iio_device_register(indio_dev);
and drop remove() callback entirely.
> +}
> +
> +static void as6200_remove(struct i2c_client *client)
> +{
> + struct as6200 *as = i2c_get_clientdata(client);
> +
> + iio_device_unregister(as->indio_dev);
> +}
...
> +
> +static const struct dev_pm_ops as6200_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(as6200_suspend, as6200_resume)
> +};
> +
> +static const struct i2c_device_id as6200_id_table[] = {
> + { "as6200" },
> + { },
As below.
> +};
> +MODULE_DEVICE_TABLE(i2c, as6200_id_table);
> +
> +static const struct of_device_id as6200_of_match[] = {
> + { .compatible = "ams,as6200" },
> + { },
No point in a , after a 'NULL' like terminator as we will
never add anything after it.
> +};
> +MODULE_DEVICE_TABLE(of, as6200_of_match);
> +
> +static struct i2c_driver as6200_driver = {
> + .driver = {
> + .name = "as6200",
> + .pm = &as6200_pm_ops,
.pm = pm_sleep_ptr(&as6200_pm_ops),
see what that function is doing to understand why (all about cleaning up
unnecessary code if the PM config doesn't require it)
> + .of_match_table = as6200_of_match,
> + },
> + .probe = as6200_probe,
> + .remove = as6200_remove,
> + .id_table = as6200_id_table,
> +};
> +module_i2c_driver(as6200_driver);
> +
> +MODULE_AUTHOR("Abdel Alkuor <alkuor@gmail.com");
> +MODULE_DESCRIPTION("AMS AS6200 Temperature Sensor");
> +MODULE_LICENSE("GPL");
On Sun, Nov 26, 2023 at 05:57:26PM +0000, Jonathan Cameron wrote:
> On Sat, 11 Nov 2023 17:05:02 -0500
> Abdel Alkuor <alkuor@gmail.com> wrote:
>
> > as6200 is a high accuracy temperature sensor of 0.0625C with a range
> > between -40 to 125 Celsius degrees. The driver implements the alert
> > trigger event in comparator mode where an alert would trigger
> > when n number of consecutive current temperature is above the upper
> > temp limit, and the alert is only cleared when the n number of
> > consecutive current temperature is below the lower temp limit.
> >
> Hi Abdel,
>
> Comments inline. Sorry it took so long for you get a review. I've been
> travelling + snowed under since returning.
> Seems like a very feature rich driver whilst still being nice and short :)
>
Hi Jonathan,
No worries. I figured as I'm following you on LinkedIn :-) I hope you
enjoyed your travel.
Thank you for your time and the thorough review. I'll address your
comments in v2.
> > The driver supports the following:
> > - show available sampling frequencey
> > - read/write sampling frequency
> > - read raw temperature
> > - read scaling factor
> > - read/write consective faults to trigger/clear an alert
> > - show available consecutive faults
> > - buffer trigger
> > - temperature alert event trigger
> >
> > https://ams.com/documents/20143/36005/AS6200_DS000449_4-00.pdf
>
> Use a formal tag in the tag block for this.
>
> >
> Datasheet: https://ams.com/documents/20143/36005/AS6200_DS000449_4-00.pdf
Will add it.
> > Signed-off-by: Abdel Alkuor <alkuor@gmail.com>
>
>
>
> > ---
> > drivers/iio/temperature/Kconfig | 9 +
> > drivers/iio/temperature/Makefile | 1 +
> > drivers/iio/temperature/as6200.c | 507 +++++++++++++++++++++++++++++++
> > 3 files changed, 517 insertions(+)
> > create mode 100644 drivers/iio/temperature/as6200.c
> >
> > diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
> > index ed384f33e0c7..f32691744952 100644
> > --- a/drivers/iio/temperature/Kconfig
> > +++ b/drivers/iio/temperature/Kconfig
> > @@ -157,5 +157,14 @@ config MAX31865
> >
> > This driver can also be build as a module. If so, the module
> > will be called max31865.
> blank line. Also should be in alphabetical order, not down the end of the file.
>
> Appending to the end causes a lot more trouble for merging multiple series than
> alphabetical order, which is one reason we always try to keep these files in order.
>
> > +config AS6200
> > + tristate "AS6200 temperature sensor"
> > + depends on I2C
> > + help
> > + If you say yes here you get support for AS6200
> > + temperature sensor chip connected via I2C.
> > +
> > + This driver can also be built as a module. If so, the module
> > + will be called as6200.
> >
> > endmenu
> > diff --git a/drivers/iio/temperature/Makefile b/drivers/iio/temperature/Makefile
> > index dfec8c6d3019..48f647c273c1 100644
> > --- a/drivers/iio/temperature/Makefile
> > +++ b/drivers/iio/temperature/Makefile
> > @@ -17,3 +17,4 @@ obj-$(CONFIG_TMP007) += tmp007.o
> > obj-$(CONFIG_TMP117) += tmp117.o
> > obj-$(CONFIG_TSYS01) += tsys01.o
> > obj-$(CONFIG_TSYS02D) += tsys02d.o
> > +obj-$(CONFIG_AS6200) += as6200.o
> Alphabetical order.
>
Will be fixed in v2.
> > diff --git a/drivers/iio/temperature/as6200.c b/drivers/iio/temperature/as6200.c
> > new file mode 100644
> > index 000000000000..a18c5be0a229
> > --- /dev/null
> > +++ b/drivers/iio/temperature/as6200.c
> > @@ -0,0 +1,507 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Driver for AMS AS6200 Temperature sensor
> > + *
> > + * Auther: Abdel Alkuor <alkuor@gmail.com>
> Spell check comments Author.
> > + */
> > +
> > +#include <linux/i2c.h>
>
> Alphabetical order, but it's common to have the iio/*
> headers as a separate block afterwards given this is an IIO driver.
>
Will be addressed in v2.
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/events.h>
> > +#include <linux/iio/trigger.h>
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/triggered_buffer.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/device.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/kstrtox.h>
> > +
> > +#define AS_TVAL_REG 0x0
>
> Probably want a more specific prefix.
> AS6200_TVAL_REG etc
>
Sounds good. I'll prefix AS6200 for the defines.
> > +#define AS_CONFIG_REG 0x1
> > +#define AS_TLOW_REG 0x2
> > +#define AS_THIGH_REG 0x3
> > +
> > +/* AS_CONFIG_REG */
> The register and field naming should make it clear enough which register
> these are in. It think that is true here so you don't need the comment.
> Excess comments are just opportunities for code rot so keep them for when
> they add significant value.
>
Will be removed in v2.
> > +#define AS_CONFIG_AL BIT(5)
> > +#define AS_CONFIG_CR GENMASK(7, 6)
> > +#define AS_CONFIG_SM BIT(8)
> > +#define AS_CONFIG_IM BIT(9)
> > +#define AS_CONFIG_POL BIT(10)
> > +#define AS_CONFIG_CF GENMASK(12, 11)
> > +
> > +#define AS_TEMP_SCALE 62500
>
> This isn't a magic number, it's an actual quantity. As such, just put the
> number inline in the code rather than use a define that just makes it harder
> to review.
>
Understood. Will be removed in v2
> > +
> > +struct as6200_freq {
> > + int val;
> > + int val2;
> > +};
>
> I don't mind the structure, but not sure it adds much over a simple 2d array of
> integers given the ordering of val and val2 is fairly natural anyway.
>
Cool. I'll use a 2d array instead.
> > +
> > +struct as6200 {
> > + struct device *dev;
> > + struct regmap *regmap;
> > + struct mutex lock;
> All locks need a comment to explain what data they are protecting.
>
A comment will be added in v2.
> > + struct iio_dev *indio_dev;
>
> You may have noticed this is almost never done in IIO drivers. It normally
> indicates a problem with the architecture. There is a natural way to go
> from iio_dev to iio_priv() so we shouldn't nee to go the other way.
>
This makes a lot of sense, not sure how I missed that. Will use iio_priv instead.
> If you did need to have a remove() (which you don't - see later) then
> should by the struct iio_dev * in the i2c_clientdata not the
> iio_priv() structure.
>
No need, remove() will be removed in v2 as you suggested below.
> > +
> > + unsigned int data[3];
> > +};
> > +
> > +static const struct as6200_freq as6200_samp_freq[4] = {
> > + {0, 250000},
>
> Trivial: Slight preference for more spaces { 0, 2500000 },
>
> > + {1, 0},
> > + {4, 0},
> > + {8, 0},
> > +};
> > +
Will added spaces in v2.
> > +static const struct iio_event_spec as6200_temp_event[] = {
> > + {
> > + .type = IIO_EV_TYPE_THRESH,
> > + .dir = IIO_EV_DIR_RISING,
> > + .mask_separate = BIT(IIO_EV_INFO_VALUE)
> > + },
> > + {
> > + .type = IIO_EV_TYPE_THRESH,
> > + .dir = IIO_EV_DIR_FALLING,
> > + .mask_separate = BIT(IIO_EV_INFO_VALUE)
> > + },
> > +};
> > +
> > +static const struct iio_chan_spec as6200_temp_channels[] = {
>
> There aren't any other channels so as6200_channels is enough.
>
Will be renamed in v2.
> > + {
> > + .type = IIO_TEMP,
> > + .indexed = 0,
> That's the 'obvious' default so don't provide it.
> > + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > + BIT(IIO_CHAN_INFO_SCALE) |
> > + BIT(IIO_CHAN_INFO_SAMP_FREQ),
> > + .scan_index = 0,
>
> With .indexed =0, this isn't used so no need to specify it here.
>
Will be addressed in v2.
> > + .scan_type = {
> > + .sign = 's',
> > + .realbits = 12,
> > + .storagebits = 16,
> > + .shift = 4,
> > + },
> > + .event_spec = as6200_temp_event,
> > + .num_event_specs = ARRAY_SIZE(as6200_temp_event),
> > + },
> > + IIO_CHAN_SOFT_TIMESTAMP(1),
> > +};
> > +
> > +static const struct regmap_config as6200_regmap_config = {
> > + .reg_bits = 8,
> > + .val_bits = 16,
> > + .max_register = 0x7F,
> > +};
> > +
> > +static int
> > +as6200_modify_config_reg(struct as6200 *as, unsigned int mask, unsigned int val)
> > +{
> > + int ret;
> > + unsigned int reg;
> > +
> > + ret = regmap_read(as->regmap, AS_CONFIG_REG, ®);
> > + if (ret)
> > + return ret;
> > +
> > + reg &= ~mask;
> > + reg |= val << (ffs(mask) - 1);
> > +
> > + return regmap_write(as->regmap, AS_CONFIG_REG, reg);
>
> regmap has functions to handle a read modify write cycle as simple as this.
> Use those instead of inventing your own. regmap_update_bits() for example
>
> They also hold the regmap lock over the whole cycle which can simplify locking
> requirements on a driver using it.
>
Sure, I'll use regmap_update_bits instead.
>
> > +}
> > +
> > +static int
> > +as6200_read_config_reg(struct as6200 *as, unsigned int mask, unsigned int *val)
>
> All the masks passed to this are constant, so it would be better to just
> put something like
>
> ret = regmap_read(as->regmap, AS_CONFIG_REG, ®);
> if (ret)
> return ret;
>
> x = FIELD_GET(reg, MASK);
>
> inline and get rid of this function entirely.
>
Sounds good.
> > +{
> > + int ret;
> > + unsigned int reg;
> > +
> > + ret = regmap_read(as->regmap, AS_CONFIG_REG, ®);
> > + if (ret)
> > + return ret;
> > +
> > + *val = (reg & mask) >> (ffs(mask) - 1);
> > +
> > + return 0;
> > +}
> > +
>
> > +
> > +static int as6200_read_event_value(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 as6200 *as = iio_device_get_drvdata(indio_dev);
> > + unsigned int reg;
> > + int ret;
> > + unsigned int temp;
> > +
> > + switch (dir) {
> > + case IIO_EV_DIR_FALLING:
> > + reg = AS_TLOW_REG;
> > + break;
> > + case IIO_EV_DIR_RISING:
> > + reg = AS_THIGH_REG;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + ret = regmap_read(as->regmap, reg, &temp);
> > + if (ret)
> > + return ret;
> > +
> > + *val = sign_extend32(temp >> 4, 11);
>
> That shift looks like it should be handled via a FIELD_GET()
> an appropriate mask.
>
Will add a temp mask to handle this.
> > +
> > + return IIO_VAL_INT;
> > +}
> > +
> > +static int as6200_write_event_value(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 as6200 *as = iio_device_get_drvdata(indio_dev);
>
> Why? You have the iio_dev so
> struct as6000 *as = iio_priv(indio_dev);
>
Yup, I misunderstood the use of iio_device_get_drvdata. Will use
iio_priv instead in v2.
> > + unsigned int temp;
> > + unsigned int reg;
> > +
> > + /*
> > + * range without applying the scaling
> > + * factor of 0.06250
> > + */
> > + if (val > 2000 || val < -640)
> > + return -EINVAL;
> > +
> > + temp = (val & 0xfff) << 4;
>
> FIELD_PREP() with appropriately defined _MASK
>
Will be addressed in v2.
> > +
> > + switch (dir) {
> > + case IIO_EV_DIR_FALLING:
> > + reg = AS_TLOW_REG;
> > + break;
> > + case IIO_EV_DIR_RISING:
> > + reg = AS_THIGH_REG;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + return regmap_write(as->regmap, reg, temp);
> > +}
> > +
> > +static irqreturn_t as6200_event_handler(int irq, void *private)
> > +{
> > + struct iio_dev *indio_dev = private;
> > + struct as6200 *as = iio_device_get_drvdata(indio_dev);
> As above.
> > + unsigned int alert;
> > + enum iio_event_direction dir;
> > + int ret;
> > +
> > + mutex_lock(&as->lock);
> guard(mutex)(&as->lock);
>
> then you don't need to do any manual unlocking.
>
This is pretty cool. I've never known this exists. Now, I know
and I'll use it :)
> > +
> > + ret = as6200_read_config_reg(as, AS_CONFIG_AL, &alert);
> > + if (ret) {
> > + mutex_unlock(&as->lock);
> > + return IRQ_NONE;
> > + }
> > +
> > + dir = alert ? IIO_EV_DIR_FALLING : IIO_EV_DIR_RISING;
> > +
> > + iio_push_event(indio_dev,
> > + IIO_EVENT_CODE(IIO_TEMP, 0, 0,
> > + dir,
> > + IIO_EV_TYPE_THRESH,
> > + 0, 0, 0),
> > + iio_get_time_ns(indio_dev));
> > +
> > + mutex_unlock(&as->lock);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
> > +static irqreturn_t as6200_trigger_handler(int irq, void *private)
> > +{
> > + struct iio_poll_func *pf = private;
> > + struct iio_dev *indio_dev = pf->indio_dev;
> > + struct as6200 *as = iio_device_get_drvdata(indio_dev);
> > + int ret;
> > +
> > + mutex_lock(&as->lock);
>
> scoped_guard(mutex, &as->lock) {
> ret = regmap_read(as->regmap, AS_TVAL_REG, &as->data[0]);
> if (ret)
> break;
>
> iio_push_to_buffers_with_timestamp(indio_dev, as->data,
> iio_get_time_ns(indio_dev));
>
> However, data isn't big enough. It should contain enough space for the data +
> a naturally aligned s64 (so 16 bytes). This interface is deeply unintuitive
> but we have been stuck with it for a long time now :(
>
I agree, the data index for each element kinda depend on the alignment.
So from user space, a user needs to be aware of such alignment to
process each data element properly.
> Also, I'm not sure why we can't just have data on the stack and if we do,
> is there a need for the lock?
>
This is a good point. Simply, an oversight from my side. I'll remove the
lock and put the data on the stack.
> }
>
> iio_trigger_notify_done()....
>
>
> > +
> > + ret = regmap_read(as->regmap, AS_TVAL_REG, &as->data[0]);
> > + if (!ret)
> > + iio_push_to_buffers_with_timestamp(indio_dev, as->data,
> > + iio_get_time_ns(indio_dev));
> > +
> > + mutex_unlock(&as->lock);
> > +
> > + iio_trigger_notify_done(indio_dev->trig);
> > +
> > + return IRQ_HANDLED;
> > +}
> > +
>
> > +
> > +static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("0.25 1 4 8");
> > +static IIO_CONST_ATTR(avail_consecutive_faults, "1 2 4 6");
> > +static IIO_DEVICE_ATTR_RW(consecutive_faults, 0);
> > +
> > +static struct attribute *as6200_event_attributes[] = {
> > + &iio_const_attr_avail_consecutive_faults.dev_attr.attr,
> > + &iio_dev_attr_consecutive_faults.dev_attr.attr,
> Custom ABI, so should be some documentation.
>
> I'm guessing a bit, but sounds like the standard ABI period
> to me.
> https://elixir.bootlin.com/linux/latest/source/Documentation/ABI/testing/sysfs-bus-iio#L1163
>
> However you will need to take into account current sampling frequency
> and provide this in seconds as per the documentation.
>
Sounds good. I'll add it under period with the proper conversions.
> It's absolutely fine to have available change with the sampling
> frequency.
>
> > + NULL,
> > +};
> > +
> > +static struct attribute *as6200_attributes[] = {
> > + &iio_const_attr_sampling_frequency_available.dev_attr.attr,
>
> For this one, use read_avail() and appropriate bitmaps.
> That both reduces manual handling of attributes and allows in kernel
> consumers to potentially access this parameter.
>
Will be addressed in v2.
> > + NULL,
> > +};
> > +
> > +static const struct attribute_group as6200_attribute_group = {
> > + .attrs = as6200_attributes,
> > +};
> > +
> > +static const struct attribute_group as6200_event_attribute_group = {
> > + .attrs = as6200_event_attributes,
> > +};
> > +
> > +static const struct iio_info as6200_temp_info = {
> > + .event_attrs = &as6200_event_attribute_group,
> > + .attrs = &as6200_attribute_group,
> > + .read_raw = &as6200_read_raw,
> > + .write_raw = &as6200_write_raw,
> > + .read_event_value = &as6200_read_event_value,
> > + .write_event_value = &as6200_write_event_value,
> > +};
> > +
> > +static int as6200_probe(struct i2c_client *client)
> > +{
> > + struct as6200 *as;
> > + struct iio_dev *indio_dev;
> > + int ret;
> > +
> > + if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> > + return -EINVAL;
> > +
> > + indio_dev = devm_iio_device_alloc(&client->dev, 0);
> > + if (!indio_dev)
> > + return -ENOMEM;
> > +
> > + as = devm_kzalloc(&client->dev, sizeof(*as), GFP_KERNEL);
> > + if (IS_ERR(as))
> > + return PTR_ERR(as);
> > +
> > + as->regmap = devm_regmap_init_i2c(client, &as6200_regmap_config);
> > + if (IS_ERR(as->regmap))
> > + return PTR_ERR(as->regmap);
> > +
> > + mutex_init(&as->lock);
> > +
> > + as->dev = &client->dev;
> > + as->indio_dev = indio_dev;
> > +
> > + iio_device_set_drvdata(indio_dev, as);
> > +
> > + indio_dev->modes = INDIO_DIRECT_MODE;
> > + indio_dev->channels = as6200_temp_channels;
> > + indio_dev->num_channels = ARRAY_SIZE(as6200_temp_channels);
> > + indio_dev->name = client->name;
>
> Encode the string directly here. The different firmware types fill client->name
> in via various fragile means. Things are safer if we don't rely on that.
>
Will encode the string directly in v2.
> > + indio_dev->info = &as6200_temp_info;
> > +
> > + ret = devm_iio_triggered_buffer_setup(as->dev, indio_dev,
> > + NULL,
> > + as6200_trigger_handler,
> > + NULL);
> > + if (ret)
> > + return ret;
> > +
> > + if (client->irq) {
> > + ret = devm_request_threaded_irq(as->dev,
> > + client->irq,
> > + NULL,
> > + &as6200_event_handler,
> > + IRQF_ONESHOT,
> > + client->name,
> > + indio_dev);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + i2c_set_clientdata(client, as);
> > +
> > + return iio_device_register(indio_dev);
>
> As only thing in remove is iio_device_unregister() use
> return devm_iio_device_register(indio_dev);
> and drop remove() callback entirely.
>
Sounds good.
> > +}
> > +
> > +static void as6200_remove(struct i2c_client *client)
> > +{
> > + struct as6200 *as = i2c_get_clientdata(client);
> > +
> > + iio_device_unregister(as->indio_dev);
> > +}
>
> ...
>
will be dropped in v2.
> > +
> > +static const struct dev_pm_ops as6200_pm_ops = {
> > + SET_SYSTEM_SLEEP_PM_OPS(as6200_suspend, as6200_resume)
> > +};
> > +
> > +static const struct i2c_device_id as6200_id_table[] = {
> > + { "as6200" },
> > + { },
> As below.
>
> > +};
> > +MODULE_DEVICE_TABLE(i2c, as6200_id_table);
> > +
> > +static const struct of_device_id as6200_of_match[] = {
> > + { .compatible = "ams,as6200" },
> > + { },
>
> No point in a , after a 'NULL' like terminator as we will
> never add anything after it.
>
Understood.
> > +};
> > +MODULE_DEVICE_TABLE(of, as6200_of_match);
> > +
> > +static struct i2c_driver as6200_driver = {
> > + .driver = {
> > + .name = "as6200",
> > + .pm = &as6200_pm_ops,
> .pm = pm_sleep_ptr(&as6200_pm_ops),
>
> see what that function is doing to understand why (all about cleaning up
> unnecessary code if the PM config doesn't require it)
>
Will be added in v2.
> > + .of_match_table = as6200_of_match,
> > + },
> > + .probe = as6200_probe,
> > + .remove = as6200_remove,
> > + .id_table = as6200_id_table,
> > +};
> > +module_i2c_driver(as6200_driver);
> > +
> > +MODULE_AUTHOR("Abdel Alkuor <alkuor@gmail.com");
> > +MODULE_DESCRIPTION("AMS AS6200 Temperature Sensor");
> > +MODULE_LICENSE("GPL");
>
@@ -157,5 +157,14 @@ config MAX31865
This driver can also be build as a module. If so, the module
will be called max31865.
+config AS6200
+ tristate "AS6200 temperature sensor"
+ depends on I2C
+ help
+ If you say yes here you get support for AS6200
+ temperature sensor chip connected via I2C.
+
+ This driver can also be built as a module. If so, the module
+ will be called as6200.
endmenu
@@ -17,3 +17,4 @@ obj-$(CONFIG_TMP007) += tmp007.o
obj-$(CONFIG_TMP117) += tmp117.o
obj-$(CONFIG_TSYS01) += tsys01.o
obj-$(CONFIG_TSYS02D) += tsys02d.o
+obj-$(CONFIG_AS6200) += as6200.o
new file mode 100644
@@ -0,0 +1,507 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for AMS AS6200 Temperature sensor
+ *
+ * Auther: Abdel Alkuor <alkuor@gmail.com>
+ */
+
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <linux/regmap.h>
+#include <linux/interrupt.h>
+#include <linux/iio/iio.h>
+#include <linux/iio/events.h>
+#include <linux/iio/trigger.h>
+#include <linux/iio/buffer.h>
+#include <linux/iio/triggered_buffer.h>
+#include <linux/iio/trigger_consumer.h>
+#include <linux/iio/sysfs.h>
+#include <linux/device.h>
+#include <linux/bitfield.h>
+#include <linux/kstrtox.h>
+
+#define AS_TVAL_REG 0x0
+#define AS_CONFIG_REG 0x1
+#define AS_TLOW_REG 0x2
+#define AS_THIGH_REG 0x3
+
+/* AS_CONFIG_REG */
+#define AS_CONFIG_AL BIT(5)
+#define AS_CONFIG_CR GENMASK(7, 6)
+#define AS_CONFIG_SM BIT(8)
+#define AS_CONFIG_IM BIT(9)
+#define AS_CONFIG_POL BIT(10)
+#define AS_CONFIG_CF GENMASK(12, 11)
+
+#define AS_TEMP_SCALE 62500
+
+struct as6200_freq {
+ int val;
+ int val2;
+};
+
+struct as6200 {
+ struct device *dev;
+ struct regmap *regmap;
+ struct mutex lock;
+ struct iio_dev *indio_dev;
+
+ unsigned int data[3];
+};
+
+static const struct as6200_freq as6200_samp_freq[4] = {
+ {0, 250000},
+ {1, 0},
+ {4, 0},
+ {8, 0},
+};
+
+static const struct iio_event_spec as6200_temp_event[] = {
+ {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_RISING,
+ .mask_separate = BIT(IIO_EV_INFO_VALUE)
+ },
+ {
+ .type = IIO_EV_TYPE_THRESH,
+ .dir = IIO_EV_DIR_FALLING,
+ .mask_separate = BIT(IIO_EV_INFO_VALUE)
+ },
+};
+
+static const struct iio_chan_spec as6200_temp_channels[] = {
+ {
+ .type = IIO_TEMP,
+ .indexed = 0,
+ .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
+ BIT(IIO_CHAN_INFO_SCALE) |
+ BIT(IIO_CHAN_INFO_SAMP_FREQ),
+ .scan_index = 0,
+ .scan_type = {
+ .sign = 's',
+ .realbits = 12,
+ .storagebits = 16,
+ .shift = 4,
+ },
+ .event_spec = as6200_temp_event,
+ .num_event_specs = ARRAY_SIZE(as6200_temp_event),
+ },
+ IIO_CHAN_SOFT_TIMESTAMP(1),
+};
+
+static const struct regmap_config as6200_regmap_config = {
+ .reg_bits = 8,
+ .val_bits = 16,
+ .max_register = 0x7F,
+};
+
+static int
+as6200_modify_config_reg(struct as6200 *as, unsigned int mask, unsigned int val)
+{
+ int ret;
+ unsigned int reg;
+
+ ret = regmap_read(as->regmap, AS_CONFIG_REG, ®);
+ if (ret)
+ return ret;
+
+ reg &= ~mask;
+ reg |= val << (ffs(mask) - 1);
+
+ return regmap_write(as->regmap, AS_CONFIG_REG, reg);
+}
+
+static int
+as6200_read_config_reg(struct as6200 *as, unsigned int mask, unsigned int *val)
+{
+ int ret;
+ unsigned int reg;
+
+ ret = regmap_read(as->regmap, AS_CONFIG_REG, ®);
+ if (ret)
+ return ret;
+
+ *val = (reg & mask) >> (ffs(mask) - 1);
+
+ return 0;
+}
+
+static int as6200_read_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int *val,
+ int *val2,
+ long mask)
+{
+ struct as6200 *as = iio_device_get_drvdata(indio_dev);
+ unsigned int reg;
+ int ret;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_RAW:
+ ret = regmap_read(as->regmap, AS_TVAL_REG, ®);
+ if (ret)
+ return ret;
+
+ *val = sign_extend32(reg >> 4, 11);
+ return IIO_VAL_INT;
+ case IIO_CHAN_INFO_SCALE:
+ *val = 0;
+ *val2 = AS_TEMP_SCALE;
+ return IIO_VAL_INT_PLUS_MICRO;
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ ret = as6200_read_config_reg(as, AS_CONFIG_CR, ®);
+ if (ret)
+ return ret;
+
+ *val = as6200_samp_freq[reg].val;
+ *val2 = as6200_samp_freq[reg].val2;
+ return IIO_VAL_INT_PLUS_MICRO;
+ default:
+ return -EINVAL;
+ }
+}
+
+static int as6200_write_raw(struct iio_dev *indio_dev,
+ struct iio_chan_spec const *chan,
+ int val,
+ int val2,
+ long mask)
+{
+ struct as6200 *as = iio_device_get_drvdata(indio_dev);
+ int i;
+
+ switch (mask) {
+ case IIO_CHAN_INFO_SAMP_FREQ:
+ for (i = 0; i < ARRAY_SIZE(as6200_samp_freq); i++) {
+ if (val == as6200_samp_freq[i].val &&
+ val2 == as6200_samp_freq[i].val2)
+ break;
+ }
+
+ if (i == ARRAY_SIZE(as6200_samp_freq))
+ return -EINVAL;
+
+ return as6200_modify_config_reg(as, AS_CONFIG_CR, i);
+ default:
+ return -EINVAL;
+ }
+}
+
+static int as6200_read_event_value(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 as6200 *as = iio_device_get_drvdata(indio_dev);
+ unsigned int reg;
+ int ret;
+ unsigned int temp;
+
+ switch (dir) {
+ case IIO_EV_DIR_FALLING:
+ reg = AS_TLOW_REG;
+ break;
+ case IIO_EV_DIR_RISING:
+ reg = AS_THIGH_REG;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ ret = regmap_read(as->regmap, reg, &temp);
+ if (ret)
+ return ret;
+
+ *val = sign_extend32(temp >> 4, 11);
+
+ return IIO_VAL_INT;
+}
+
+static int as6200_write_event_value(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 as6200 *as = iio_device_get_drvdata(indio_dev);
+ unsigned int temp;
+ unsigned int reg;
+
+ /*
+ * range without applying the scaling
+ * factor of 0.06250
+ */
+ if (val > 2000 || val < -640)
+ return -EINVAL;
+
+ temp = (val & 0xfff) << 4;
+
+ switch (dir) {
+ case IIO_EV_DIR_FALLING:
+ reg = AS_TLOW_REG;
+ break;
+ case IIO_EV_DIR_RISING:
+ reg = AS_THIGH_REG;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ return regmap_write(as->regmap, reg, temp);
+}
+
+static irqreturn_t as6200_event_handler(int irq, void *private)
+{
+ struct iio_dev *indio_dev = private;
+ struct as6200 *as = iio_device_get_drvdata(indio_dev);
+ unsigned int alert;
+ enum iio_event_direction dir;
+ int ret;
+
+ mutex_lock(&as->lock);
+
+ ret = as6200_read_config_reg(as, AS_CONFIG_AL, &alert);
+ if (ret) {
+ mutex_unlock(&as->lock);
+ return IRQ_NONE;
+ }
+
+ dir = alert ? IIO_EV_DIR_FALLING : IIO_EV_DIR_RISING;
+
+ iio_push_event(indio_dev,
+ IIO_EVENT_CODE(IIO_TEMP, 0, 0,
+ dir,
+ IIO_EV_TYPE_THRESH,
+ 0, 0, 0),
+ iio_get_time_ns(indio_dev));
+
+ mutex_unlock(&as->lock);
+
+ return IRQ_HANDLED;
+}
+
+static irqreturn_t as6200_trigger_handler(int irq, void *private)
+{
+ struct iio_poll_func *pf = private;
+ struct iio_dev *indio_dev = pf->indio_dev;
+ struct as6200 *as = iio_device_get_drvdata(indio_dev);
+ int ret;
+
+ mutex_lock(&as->lock);
+
+ ret = regmap_read(as->regmap, AS_TVAL_REG, &as->data[0]);
+ if (!ret)
+ iio_push_to_buffers_with_timestamp(indio_dev, as->data,
+ iio_get_time_ns(indio_dev));
+
+ mutex_unlock(&as->lock);
+
+ iio_trigger_notify_done(indio_dev->trig);
+
+ return IRQ_HANDLED;
+}
+
+static ssize_t
+consecutive_faults_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+ struct as6200 *as = iio_device_get_drvdata(indio_dev);
+ unsigned int cf;
+ int ret;
+
+ ret = as6200_read_config_reg(as, AS_CONFIG_CF, &cf);
+ if (ret)
+ return ret;
+
+ return sprintf(buf, "%u\n", cf ? cf * 2 : 1);
+}
+
+static ssize_t
+consecutive_faults_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct iio_dev *indio_dev = dev_to_iio_dev(dev);
+ struct as6200 *as = iio_device_get_drvdata(indio_dev);
+ unsigned int cf;
+ int ret;
+
+ ret = kstrtouint(buf, 0, &cf);
+ if (ret)
+ return ret;
+
+ switch (cf) {
+ case 1:
+ cf = 0;
+ break;
+ case 2:
+ case 4:
+ case 6:
+ cf /= 2;
+ break;
+ default:
+ return -EINVAL;
+ }
+
+ ret = as6200_modify_config_reg(as, AS_CONFIG_CF, cf);
+ if (ret)
+ return ret;
+
+ return count;
+}
+
+static IIO_CONST_ATTR_SAMP_FREQ_AVAIL("0.25 1 4 8");
+static IIO_CONST_ATTR(avail_consecutive_faults, "1 2 4 6");
+static IIO_DEVICE_ATTR_RW(consecutive_faults, 0);
+
+static struct attribute *as6200_event_attributes[] = {
+ &iio_const_attr_avail_consecutive_faults.dev_attr.attr,
+ &iio_dev_attr_consecutive_faults.dev_attr.attr,
+ NULL,
+};
+
+static struct attribute *as6200_attributes[] = {
+ &iio_const_attr_sampling_frequency_available.dev_attr.attr,
+ NULL,
+};
+
+static const struct attribute_group as6200_attribute_group = {
+ .attrs = as6200_attributes,
+};
+
+static const struct attribute_group as6200_event_attribute_group = {
+ .attrs = as6200_event_attributes,
+};
+
+static const struct iio_info as6200_temp_info = {
+ .event_attrs = &as6200_event_attribute_group,
+ .attrs = &as6200_attribute_group,
+ .read_raw = &as6200_read_raw,
+ .write_raw = &as6200_write_raw,
+ .read_event_value = &as6200_read_event_value,
+ .write_event_value = &as6200_write_event_value,
+};
+
+static int as6200_probe(struct i2c_client *client)
+{
+ struct as6200 *as;
+ struct iio_dev *indio_dev;
+ int ret;
+
+ if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
+ return -EINVAL;
+
+ indio_dev = devm_iio_device_alloc(&client->dev, 0);
+ if (!indio_dev)
+ return -ENOMEM;
+
+ as = devm_kzalloc(&client->dev, sizeof(*as), GFP_KERNEL);
+ if (IS_ERR(as))
+ return PTR_ERR(as);
+
+ as->regmap = devm_regmap_init_i2c(client, &as6200_regmap_config);
+ if (IS_ERR(as->regmap))
+ return PTR_ERR(as->regmap);
+
+ mutex_init(&as->lock);
+
+ as->dev = &client->dev;
+ as->indio_dev = indio_dev;
+
+ iio_device_set_drvdata(indio_dev, as);
+
+ indio_dev->modes = INDIO_DIRECT_MODE;
+ indio_dev->channels = as6200_temp_channels;
+ indio_dev->num_channels = ARRAY_SIZE(as6200_temp_channels);
+ indio_dev->name = client->name;
+ indio_dev->info = &as6200_temp_info;
+
+ ret = devm_iio_triggered_buffer_setup(as->dev, indio_dev,
+ NULL,
+ as6200_trigger_handler,
+ NULL);
+ if (ret)
+ return ret;
+
+ if (client->irq) {
+ ret = devm_request_threaded_irq(as->dev,
+ client->irq,
+ NULL,
+ &as6200_event_handler,
+ IRQF_ONESHOT,
+ client->name,
+ indio_dev);
+ if (ret)
+ return ret;
+ }
+
+ i2c_set_clientdata(client, as);
+
+ return iio_device_register(indio_dev);
+}
+
+static void as6200_remove(struct i2c_client *client)
+{
+ struct as6200 *as = i2c_get_clientdata(client);
+
+ iio_device_unregister(as->indio_dev);
+}
+
+static int __maybe_unused as6200_suspend(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct as6200 *as = i2c_get_clientdata(client);
+
+ if (client->irq)
+ disable_irq(client->irq);
+
+ return as6200_modify_config_reg(as, AS_CONFIG_SM, 1);
+}
+
+static int __maybe_unused as6200_resume(struct device *dev)
+{
+ struct i2c_client *client = to_i2c_client(dev);
+ struct as6200 *as = i2c_get_clientdata(client);
+
+ if (client->irq)
+ enable_irq(client->irq);
+
+ return as6200_modify_config_reg(as, AS_CONFIG_SM, 0);
+}
+
+static const struct dev_pm_ops as6200_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(as6200_suspend, as6200_resume)
+};
+
+static const struct i2c_device_id as6200_id_table[] = {
+ { "as6200" },
+ { },
+};
+MODULE_DEVICE_TABLE(i2c, as6200_id_table);
+
+static const struct of_device_id as6200_of_match[] = {
+ { .compatible = "ams,as6200" },
+ { },
+};
+MODULE_DEVICE_TABLE(of, as6200_of_match);
+
+static struct i2c_driver as6200_driver = {
+ .driver = {
+ .name = "as6200",
+ .pm = &as6200_pm_ops,
+ .of_match_table = as6200_of_match,
+ },
+ .probe = as6200_probe,
+ .remove = as6200_remove,
+ .id_table = as6200_id_table,
+};
+module_i2c_driver(as6200_driver);
+
+MODULE_AUTHOR("Abdel Alkuor <alkuor@gmail.com");
+MODULE_DESCRIPTION("AMS AS6200 Temperature Sensor");
+MODULE_LICENSE("GPL");