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@xxxxxxxxx> 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@xxxxxxxxx> > > > > 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@xxxxxxxxx> > > + */ > > + > > +#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@xxxxxxxxx"); > > +MODULE_DESCRIPTION("AMS AS6200 Temperature Sensor"); > > +MODULE_LICENSE("GPL"); >
