Re: [PATCH v2 2/2] iio: temperature: Add support for AMS AS6200

Abdel Alkuor
2023-12-05T02:16:53Z

On Mon, Dec 04, 2023 at 01:50:14PM +0000, Jonathan Cameron wrote:
> On Fri, 1 Dec 2023 23:16:51 -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
> > consecutive faults are converted into periods, high/low temperature
> > thresholds require to be above/below the set limit for n seconds for
> > the alert to be triggered/cleared. The alert is only cleared when the
> > current temperature is below the low temperature threshold for n seconds.
> >
> > The driver supports the following:
> > - show available sampling frequencey
> > - read/write sampling frequency
> > - read raw temperature
> > - read scaling factor
> > - read/write temperature period that needs to be met for low/high
> > temperature thresholds to trigger an alert
> > - show available temperature period thresholds
> > - buffer trigger
> > - temperature alert event trigger
>
> Hi Abdel,
>
> A few comments inline. Looking good in general.
>
Hi Jonathon,

Thank you for your time. I have a couple _silly_ questions about the tag
and returning from if else. Other than that, your comments will be addressed
in v3.
> >
> > Datasheet: https://ams.com/documents/20143/36005/AS6200_DS000449_4-00.pdf
> >
>
> No blank line here. Tags block (and Datasheet is a tag) never has blank lines
> as that breaks some existing tooling.
>
Understood.

P.S. when running checkpatch.pl on this patch, I get the following warning:

WARNING: Unknown link reference 'Datasheet:', use 'Link:' or 'Closes:' instead

should I use Link instead?
> > Signed-off-by: Abdel Alkuor <alkuor@gmail.com>
> >
> > What: /sys/bus/iio/devices/iio:deviceX/in_filter_notch_center_frequency
> > diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
> > index ed384f33e0c7..a0ffbc77e623 100644
> > --- a/drivers/iio/temperature/Kconfig
> > +++ b/drivers/iio/temperature/Kconfig
> > @@ -4,6 +4,17 @@
> > #
> > menu "Temperature sensors"
> >
> > +config AS6200
> > + tristate "AS6200 temperature sensor"
> > + depends on I2C
> > + select REGMAP_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.
> > +
> > config IQS620AT_TEMP
> > tristate "Azoteq IQS620AT temperature sensor"
> > depends on MFD_IQS62X || COMPILE_TEST
> > @@ -157,5 +168,4 @@ config MAX31865
> >
> > This driver can also be build as a module. If so, the module
> > will be called max31865.
> > -
> Stray change.
>
Ops, I fixed it locally and forgot to regenerate a patch after. Will be fixed
in v3
> > endmenu
>
> > diff --git a/drivers/iio/temperature/as6200.c b/drivers/iio/temperature/as6200.c
> > new file mode 100644
> > index 000000000000..7fcc785871d8
> > --- /dev/null
> > +++ b/drivers/iio/temperature/as6200.c
> > @@ -0,0 +1,493 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Driver for AMS AS6200 Temperature sensor
> > + *
> > + * Author: Abdel Alkuor <alkuor@gmail.com>
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/device.h>
> > +#include <linux/i2c.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/kstrtox.h>
> > +#include <linux/module.h>
> > +#include <linux/regmap.h>
> > +
> > +#include <linux/iio/buffer.h>
> > +#include <linux/iio/events.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/iio/trigger.h>
> > +#include <linux/iio/triggered_buffer.h>
> > +#include <linux/iio/trigger_consumer.h>
> > +
> > +#define AS6200_TVAL_REG 0x0
> > +#define AS6200_CONFIG_REG 0x1
> > +#define AS6200_TLOW_REG 0x2
> > +#define AS6200_THIGH_REG 0x3
> > +
> > +#define AS6200_CONFIG_AL BIT(5)
> > +#define AS6200_CONFIG_CR GENMASK(7, 6)
> > +#define AS6200_CONFIG_SM BIT(8)
> > +#define AS6200_CONFIG_IM BIT(9)
> > +#define AS6200_CONFIG_POL BIT(10)
> > +#define AS6200_CONFIG_CF GENMASK(12, 11)
> > +
> > +#define AS6200_TEMP_MASK GENMASK(15, 4)
> > +
> > +struct as6200 {
> > + struct regmap *regmap;
> > + struct mutex lock; /* Prevent concurrent temp fault processing */
>
> Why does it matter? What might cause such processing?
>
Good point. I had a misunderstanding how the interrupts work and I did
some reading and realized that the interrupt is disabled until the
buttom half of the interrupt is completed as oneshot type is used.
I'll remove it in v3.
> > +};
> > +
> > +static const int as6200_samp_freq[4][2] = {
> > + { 0, 250000 },
> > + { 1, 0 },
> > + { 4, 0 },
> > + { 8, 0 }
> > +};
> > +
> > +/* Consective faults converted to period */
> > +static const int as6200_temp_thresh_periods[4][4][2] = {
> > + { { 4, 0 }, { 8, 0 }, { 16, 0 }, { 24, 0 } },
> > + { { 1, 0 }, { 2, 0 }, { 4, 0 }, { 6, 0 } },
> > + { { 0, 250000 }, { 0, 500000 }, { 1, 0 }, { 2, 0} },
> > + { { 0, 125000 }, { 0, 250000 }, { 0, 500000 }, { 0, 750000 } }
>
> I'd suggest naming the first column at least (which is CR I think?)
>
> So define an enum and
> enum {
> AS6200_CR_0_25HZ = 0,
> AS6200_CR_1HZ = 1,
> AS6200_CR_4HZ = 2,
> AS6200_CR_8HZ = 3,
> };
> And use that for the samp freq entries above, so that they clearly relate
> to the rows of this arram
> [AS6200_CR_0_25HZ] = { { 4, 0 }, { 8, 0 }, { 16, 0 }, { 24, 0 } },
> You could take it further and use an enum for CF as well.
>
> [AS6200_CR_0_25HZ] = {
> [AS6200_CF_1] = { 4, 0 },
> [AS6200_CF_2] = { 8, 0 },
> [AS6200_CF_4] = { 16, 0 },
> [AS6200_CF_6] = { 24, 0 },
> },
> [AS6200_CR_1_HZ] = {
> [AS6200_CF_1] = { 1, 0 },
> [AS6200_CF_2] = { 2, 0 },
> ...
> }
> etc which makes it clear where all the numbers come.
>
> > +};
>
Good suggestion. I'll fix it in v3.
>
> > +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_priv(indio_dev);
> > + unsigned int reg;
> > + unsigned int tmp;
> > + int ret;
> > + u8 cf;
> > + u8 cr;
> > +
> > + switch (dir) {
> > + case IIO_EV_DIR_FALLING:
> > + reg = AS6200_TLOW_REG;
> > + break;
> > + case IIO_EV_DIR_RISING:
> > + reg = AS6200_THIGH_REG;
> > + break;
> > + case IIO_EV_DIR_EITHER:
> > + reg = AS6200_CONFIG_REG;
> > + break;
> > + default:
> > + return -EINVAL;
> > + }
> > +
> > + ret = regmap_read(as->regmap, reg, &tmp);
> > + if (ret)
> > + return ret;
> > +
> > + if (info == IIO_EV_INFO_VALUE) {
> > + *val = sign_extend32(FIELD_GET(AS6200_TEMP_MASK, tmp), 11);
> > + ret = IIO_VAL_INT;
> return here.
>
> > + } else {
> > + cf = FIELD_GET(AS6200_CONFIG_CF, tmp);
> > + cr = FIELD_GET(AS6200_CONFIG_CR, tmp);
> > + *val = as6200_temp_thresh_periods[cr][cf][0];
> > + *val2 = as6200_temp_thresh_periods[cr][cf][1];
> > + ret = IIO_VAL_INT_PLUS_MICRO;
>
> and here. If there is nothing more to be done, it simplifies the code
> flow being read to just return as quick as possible.
>
I did it as you mentioned, and when running check_patch.pl, it gives back a
warning that else is not needed here because of the return in the if
statement. So I opted into using ret instead, should I remove the else or ignore
the warning?
> > + }
> > +
> > + return ret;
> > +}
>
> > +static irqreturn_t as6200_event_handler(int irq, void *private)
> > +{
> > + struct iio_dev *indio_dev = private;
> > + struct as6200 *as = iio_priv(indio_dev);
> > + unsigned int alert;
> > + enum iio_event_direction dir;
> > + int ret;
> > +
> > + guard(mutex)(&as->lock);
> What data are we protecting here?
>
No data actually. As I mentioned prior, will be dropped in v3.
> > +
> > + ret = regmap_read(as->regmap, AS6200_CONFIG_REG, &alert);
> > + if (ret)
> > + return IRQ_NONE;
> > +
> > + alert = FIELD_GET(AS6200_CONFIG_AL, alert);
> > +
> > + 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));
> > +
> > + 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_priv(indio_dev);
> > + int ret;
> > + u8 data[16];
>
> I'd make this much more explicit and make sure you zero it to avoid leaking
> data.
>
> struct data {
> u8 channel;
> s64 timestamp __aligned(8);
> };
>
> memset(&data, 0, sizeof(data)); /* Ensures the holes are zero filled */
>
My system 1 thinking got me here as I tested it and all holes
were set to 0. Will be fixed in v3.
> Also, avoid the casting mess and read into a local variable that is an unsigned int
> and copy it to the struct data if no error.
> > +
> > + ret = regmap_read(as->regmap, AS6200_TVAL_REG, (unsigned int *)data);
> > + if (!ret)
> Whilst it's more lines, greatly prefer to see error paths out of line and good
> paths inline (so no if (!ret))
>
> if (ret)
> goto done;
>
> iio_push...
>
> done:
> ...
>
> May seem silly but when reviewing a lot of code, keeping things looking "normal"
> is a great benefit!
>
Makes sense. Will be fixed in v3.
> > + iio_push_to_buffers_with_timestamp(indio_dev, data,
> > + iio_get_time_ns(indio_dev));
> > +
> > + iio_trigger_notify_done(indio_dev->trig);
> > +
> > + return IRQ_HANDLED;
> > +}
>
> ...
>
> > +
> > +static int __maybe_unused as6200_suspend(struct device *dev)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > + struct as6200 *as = iio_priv(i2c_get_clientdata(client));
> > +
> > + if (client->irq)
> > + disable_irq(client->irq);
> > +
> > + return regmap_update_bits(as->regmap, AS6200_CONFIG_REG,
> > + AS6200_CONFIG_SM, AS6200_CONFIG_SM);
> > +}
> > +
> > +static int __maybe_unused as6200_resume(struct device *dev)
> > +{
> > + struct i2c_client *client = to_i2c_client(dev);
> > + struct as6200 *as = iio_priv(i2c_get_clientdata(client));
> > +
> > + if (client->irq)
> > + enable_irq(client->irq);
>
> I would normally expect suspend and resume to be mirror images. If that doesn't
> make sense for some reason and we do need to do the irq handling
> before the register write in both cases then add a comment.
No reason. Will be fixed in v3.
>
> > +
> > + return regmap_update_bits(as->regmap, AS6200_CONFIG_REG,
> > + AS6200_CONFIG_SM, 0);
> > +}
> > +
> > +static const struct dev_pm_ops as6200_pm_ops = {
>
> DEFINE_SIMPLE_DEV_PM_OPS()
>
Will be fixed in v3.
>
> > + 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 = pm_sleep_ptr(&as6200_pm_ops),
> > + .of_match_table = as6200_of_match,
> > + },
> > + .probe = as6200_probe,
> > + .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");
>