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