Hello Florian,
> As requested we can also provide a link to the datasheet of the chip:
> http://ams.com/eng/Products/Environmental-Sensors/Temperature-Sensors
> We also tried to cover all points (removed all debug msgs, simpler IRQ
> pin config, etc.). Still all the show/store functions for each
> configuration bit of the sensor are in. This was a request by our product
> management. If you suggest to take the show/store functions out
> completely for the driver version in the kernel, we would also be fine
> with that. We will give them out on request for customers then.
looks a lot better; GPL copyright header missing and private API needs to
be documented (or removed, but it really is just for debug?)
more comments below
regards, p.
> Signed-off-by: Elitsa Polizoeva <elitsa.polizo...@ams.com>
> Signed-off-by: Florian Lobmaier <florian.lobma...@ams.com>
> ---
> diff --git a/drivers/iio/temperature/Kconfig b/drivers/iio/temperature/Kconfig
> index 21feaa4..d82e80f 100644
> --- a/drivers/iio/temperature/Kconfig
> +++ b/drivers/iio/temperature/Kconfig
> @@ -3,6 +3,17 @@
> #
> menu "Temperature sensors"
>
> +config AS6200
> + tristate "ams AG AS6200 temperature sensor"
> + depends on I2C
> + select REGMAP_I2C
> + help
> + If you say yes here you get support for the AS6200 temperature
> + sensor from ams AG.
> +
> + This driver can also be built as a module. If so, the module will
> + be called as6200.
> +
> config MLX90614
> tristate "MLX90614 contact-less infrared sensor"
> depends on I2C
> diff --git a/drivers/iio/temperature/Makefile
> b/drivers/iio/temperature/Makefile
> index 40710a8..c0c9a9a 100644
> --- a/drivers/iio/temperature/Makefile
> +++ b/drivers/iio/temperature/Makefile
> @@ -2,5 +2,6 @@
> # Makefile for industrial I/O temperature drivers
> #
>
> +obj-$(CONFIG_AS6200) += as6200.o
> obj-$(CONFIG_MLX90614) += mlx90614.o
> obj-$(CONFIG_TMP006) += tmp006.o
> diff --git a/drivers/iio/temperature/as6200.c
> b/drivers/iio/temperature/as6200.c
> new file mode 100644
> index 0000000..289e950
> --- /dev/null
> +++ b/drivers/iio/temperature/as6200.c
> @@ -0,0 +1,789 @@
there needs to be a GPL copyright header, not sure if MODULE_LICENSE()
below is enough
I find it useful to state the I2C 7-bit chip address here as well
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/init.h>
> +#include <linux/slab.h>
> +#include <linux/i2c.h>
> +#include <linux/err.h>
> +#include <linux/types.h>
> +#include <linux/irq.h>
> +#include <linux/interrupt.h>
> +#include <linux/gpio.h>
> +#include <linux/of_gpio.h>
> +#include <linux/iio/iio.h>
> +#include <linux/iio/sysfs.h>
> +#include <linux/iio/events.h>
> +#include <linux/regmap.h>
> +
> +static const int as6200_conv_rates[4][2] = { {4, 0}, {1, 0},
> + {0, 250000}, {0, 125000} };
> +
> +static const char * const as6200_consec_faults[] = { "1", "2", "4", "6" };
> +
> +/* AS6200 registers */
> +#define AS6200_REG_TVAL 0x00
> +#define AS6200_REG_CONFIG 0x01
> +#define AS6200_REG_TLOW 0x02
> +#define AS6200_REG_THIGH 0x03
> +#define AS6200_MAX_REGISTER 0x03
> +
> +#define AS6200_CONFIG_DW_MASK BIT(4)
> +#define AS6200_CONFIG_DW_SHIFT 4
> +#define AS6200_CONFIG_AL_MASK BIT(5)
> +#define AS6200_CONFIG_AL_SHIFT 5
> +#define AS6200_CONFIG_CR_MASK GENMASK(7, 6)
> +#define AS6200_CONFIG_CR_SHIFT 6
> +#define AS6200_CONFIG_SM_MASK BIT(8)
> +#define AS6200_CONFIG_SM_SHIFT 8
> +#define AS6200_CONFIG_IM_MASK BIT(9)
> +#define AS6200_CONFIG_IM_SHIFT 9
> +#define AS6200_CONFIG_POL_MASK BIT(10)
> +#define AS6200_CONFIG_POL_SHIFT 10
> +#define AS6200_CONFIG_CF_MASK GENMASK(12, 11)
> +#define AS6200_CONFIG_CF_SHIFT 11
> +#define AS6200_CONFIG_SS_MASK BIT(15)
> +#define AS6200_CONFIG_SS_SHIFT 15
> +
> +struct as6200_data {
> + struct i2c_client *client;
> + struct regmap *regmap;
> +};
> +
> +static const struct regmap_range as6200_readable_ranges[] = {
> + regmap_reg_range(AS6200_REG_TVAL, AS6200_REG_THIGH),
> +};
> +
> +static const struct regmap_access_table as6200_readable_table = {
> + .yes_ranges = as6200_readable_ranges,
> + .n_yes_ranges = ARRAY_SIZE(as6200_readable_ranges),
> +};
> +
> +static const struct regmap_range as6200_writable_ranges[] = {
> + regmap_reg_range(AS6200_REG_CONFIG, AS6200_REG_THIGH),
> +};
> +
> +static const struct regmap_access_table as6200_writable_table = {
> + .yes_ranges = as6200_writable_ranges,
> + .n_yes_ranges = ARRAY_SIZE(as6200_writable_ranges),
> +};
> +
> +static const struct regmap_range as6200_volatile_ranges[] = {
> + regmap_reg_range(AS6200_REG_TVAL, AS6200_REG_TVAL),
> +};
> +
> +static const struct regmap_access_table as6200_volatile_table = {
> + .yes_ranges = as6200_volatile_ranges,
> + .n_yes_ranges = ARRAY_SIZE(as6200_volatile_ranges),
> +};
> +
> +static const struct regmap_config as6200_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 16,
> + .max_register = AS6200_MAX_REGISTER,
> + .cache_type = REGCACHE_RBTREE,
> + .rd_table = &as6200_readable_table,
> + .wr_table = &as6200_writable_table,
> + .volatile_table = &as6200_volatile_table,
> +};
> +
> +static int as6200_read_raw(struct iio_dev *indio_dev,
> + struct iio_chan_spec const *channel, int *val,
> + int *val2, long mask)
> +{
> + struct as6200_data *data = iio_priv(indio_dev);
> + int cr;
> + s32 ret;
> + int err = 0;
err initialization not needed
> + unsigned int reg_val = 0;
reg_val initialization not needed
> +
> + switch (mask) {
> + case IIO_CHAN_INFO_RAW:
> + case IIO_CHAN_INFO_PROCESSED:
I'd suggest the following to save some lines and simplity control flow
if (channel->type != IIO_TEMP)
return -EINVAL
err = regmap_read(data->regmap, AS6200_REG_TVAL, ...
return IIO_VAL_INT;
(and similar below)
> + if (channel->type == IIO_TEMP) {
> + err = regmap_read(data->regmap, AS6200_REG_TVAL,
> + ®_val);
> + if (err < 0)
> + return err;
> + ret = reg_val;
why this assignment? could do
ret = sign_extend32(reg_val, 15) >> 4;
> + ret = sign_extend32(ret, 15) >> 4;
> + if (mask == IIO_CHAN_INFO_PROCESSED)
> + *val = (ret * 625) / 10000;
> + else
> + *val = ret;
> + } else {
> + break;
> + }
> + return IIO_VAL_INT;
> + case IIO_CHAN_INFO_SCALE:
> + if (channel->type == IIO_TEMP) {
> + *val = 62;
> + *val2 = 500000;
> + } else {
> + break;
> + }
> + return IIO_VAL_INT_PLUS_MICRO;
> + case IIO_CHAN_INFO_SAMP_FREQ:
> + if (channel->type == IIO_TEMP) {
> + err = regmap_read(data->regmap, AS6200_REG_CONFIG,
> + ®_val);
> + if (err < 0)
> + return err;
> + cr = (reg_val & AS6200_CONFIG_CR_MASK)
> + >> AS6200_CONFIG_CR_SHIFT;
> + *val = as6200_conv_rates[cr][0];
> + *val2 = as6200_conv_rates[cr][1];
> + } else {
> + break;
> + }
> + return IIO_VAL_INT_PLUS_MICRO;
> + default:
> + break;
> + }
> + 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_data *data = iio_priv(indio_dev);
> + int i;
> + unsigned int config_val;
> + int err = 0;
> +
> + if (mask != IIO_CHAN_INFO_SAMP_FREQ)
> + return -EINVAL;
> +
> + for (i = 0; i < ARRAY_SIZE(as6200_conv_rates); i++) {
> + if ((val == as6200_conv_rates[i][0]) &&
> + (val2 == as6200_conv_rates[i][1])) {
parenthesis not needed
> + err = regmap_read(data->regmap,
> + AS6200_REG_CONFIG, &config_val);
> + if (err < 0)
> + return err;
> + config_val &= ~AS6200_CONFIG_CR_MASK;
> + config_val |= i << AS6200_CONFIG_CR_SHIFT;
> +
> + return regmap_write(data->regmap, AS6200_REG_CONFIG,
> + config_val);
> + }
> + }
> + return -EINVAL;
> +}
> +
> +static irqreturn_t as6200_alert_isr(int irq, void *dev_id)
> +{
> + struct iio_dev *indio_dev = dev_id;
> +
> + iio_push_event(indio_dev,
> + IIO_UNMOD_EVENT_CODE(IIO_TEMP,
> + 0,
> + IIO_EV_TYPE_THRESH,
> + IIO_EV_DIR_EITHER),
> + iio_get_time_ns());
> + return IRQ_HANDLED;
> +}
> +
> +static int as6200_setup_irq(struct iio_dev *indio_dev, bool set_gpio, u8 pol)
> +{
> + int err;
> + struct as6200_data *data = iio_priv(indio_dev);
> + struct device *dev = &data->client->dev;
> + int irq_trig;
> +
set_gpio not used?
pol could be bool?
> + if (pol == 1)
> + irq_trig = IRQF_TRIGGER_RISING;
> + else
> + irq_trig = IRQF_TRIGGER_FALLING;
> +
> + err = request_irq(data->client->irq, as6200_alert_isr, irq_trig,
> + "as6200", dev);
> + if (err) {
> + dev_err(dev, "error requesting irq %d\n", err);
> + return err;
> + }
> +
> + return 0;
> +}
> +
> +static ssize_t as6200_show_thigh(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
maybe common code could be split out
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct as6200_data *data = iio_priv(indio_dev);
> + unsigned int reg_val = 0;
> + int err = 0;
init not needed
> + int val;
> +
> + err = regmap_read(data->regmap, AS6200_REG_THIGH, ®_val);
> + if (err < 0)
> + return err;
> + reg_val = reg_val >> 4;
> + val = (625 * reg_val) / 10000;
> + return sprintf(buf, "%d%cC\n", val, (unsigned char)(248));
this tries to outputs the extended ASCII ° character (degree), this makes
little sense, most terminals are utf-8 and I'm not sure what the policy is
WRT non-ASCII
> +}
> +
> +static ssize_t as6200_show_tlow(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct as6200_data *data = iio_priv(indio_dev);
> + unsigned int reg_val = 0x00;
> + int err = 0;
> + int val;
> +
> + err = regmap_read(data->regmap, AS6200_REG_TLOW, ®_val);
> + if (err < 0)
> + return err;
> + reg_val = reg_val >> 4;
> + val = (625 * reg_val) / 10000;
> + return sprintf(buf, "%d%cC\n", val, (unsigned char)(248));
> +}
> +
> +static ssize_t as6200_show_thigh_reg(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct as6200_data *data = iio_priv(indio_dev);
> + unsigned int reg_val = 0;
> + int err = 0;
> +
> + err = regmap_read(data->regmap, AS6200_REG_THIGH, ®_val);
> + if (err < 0)
> + return err;
> + return sprintf(buf, "%hX\n", reg_val);
> +}
> +
> +static ssize_t as6200_show_tlow_reg(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct as6200_data *data = iio_priv(indio_dev);
> + unsigned int reg_val = 0x00;
> + int err = 0;
> +
> + err = regmap_read(data->regmap, AS6200_REG_TLOW, ®_val);
> + if (err < 0)
> + return err;
> + return sprintf(buf, "%hX\n", reg_val);
> +}
> +
> +static ssize_t as6200_show_config(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct as6200_data *data = iio_priv(indio_dev);
> + unsigned int reg_val = 0x00;
> + int err = 0;
> +
> + err = regmap_read(data->regmap, AS6200_REG_CONFIG, ®_val);
> + if (err < 0)
> + return err;
> + return sprintf(buf, "%hX\n", reg_val);
> +}
> +
> +static ssize_t as6200_show_al(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct as6200_data *data = iio_priv(indio_dev);
> + unsigned int reg_val = 0x00;
> + int err = 0;
> + u16 pol = 0;
init not needed
> +
> + err = regmap_read(data->regmap, AS6200_REG_CONFIG, ®_val);
> + if (err < 0)
> + return err;
> + pol = (reg_val & AS6200_CONFIG_POL_MASK) >> AS6200_CONFIG_POL_SHIFT;
> + reg_val = (reg_val & AS6200_CONFIG_AL_MASK) >> AS6200_CONFIG_AL_SHIFT;
> + if (pol == 1) {
> + if (reg_val == 0)
> + return sprintf(buf, "off\n");
> + else if (reg_val == 1)
> + return sprintf(buf, "on\n");
AL_MASK is a bit, I don't see how reg_val can end up different from 0/1
maybe use a bool for pol?
> + else
> + return sprintf(buf, "invalid\n");
> + } else if (pol == 0) {
> + if (reg_val == 0)
> + return sprintf(buf, "on\n");
> + else if (reg_val == 1)
> + return sprintf(buf, "off\n");
> + else
> + return sprintf(buf, "invalid\n");
> + } else
> + return sprintf(buf, "invalid\n");
> +}
> +
> +static ssize_t as6200_show_sm(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct as6200_data *data = iio_priv(indio_dev);
> + unsigned int reg_val = 0x00;
> + int err = 0;
> +
> + err = regmap_read(data->regmap, AS6200_REG_CONFIG, ®_val);
> + if (err < 0)
> + return err;
> + return sprintf(buf, "%hX\n", (reg_val & AS6200_CONFIG_SM_MASK)
> + >> AS6200_CONFIG_SM_SHIFT);
> +}
> +
> +static ssize_t as6200_show_im(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct as6200_data *data = iio_priv(indio_dev);
> + unsigned int reg_val = 0x00;
> + int err = 0;
> +
> + err = regmap_read(data->regmap, AS6200_REG_CONFIG, ®_val);
> + if (err < 0)
> + return err;
> + return sprintf(buf, "%hX\n", (reg_val & AS6200_CONFIG_IM_MASK)
> + >> AS6200_CONFIG_IM_SHIFT);
> +}
> +
> +static ssize_t as6200_show_pol(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct as6200_data *data = iio_priv(indio_dev);
> + unsigned int reg_val = 0x00;
> + int err = 0;
> +
> + err = regmap_read(data->regmap, AS6200_REG_CONFIG, ®_val);
> + if (err < 0)
> + return err;
> + return sprintf(buf, "%hX\n", (reg_val & AS6200_CONFIG_POL_MASK)
> + >> AS6200_CONFIG_POL_SHIFT);
> +}
> +
> +static ssize_t as6200_show_cf(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct as6200_data *data = iio_priv(indio_dev);
> + unsigned int reg_val = 0x00;
> + int err = 0;
> +
> + err = regmap_read(data->regmap, AS6200_REG_CONFIG, ®_val);
> + if (err < 0)
> + return err;
> + reg_val = (reg_val & AS6200_CONFIG_CF_MASK) >> AS6200_CONFIG_CF_SHIFT;
could index into consec_faults array of strings to avoid the switch
statement?
> + switch (reg_val) {
> + case 0:
> + return sprintf(buf, "1\n");
> + case 1:
> + return sprintf(buf, "2\n");
> + case 2:
> + return sprintf(buf, "4\n");
> + case 3:
> + return sprintf(buf, "6\n");
> + }
> + return sprintf(buf, "invalid\n");
> +}
> +
> +static ssize_t as6200_show_ss(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct as6200_data *data = iio_priv(indio_dev);
> + unsigned int reg_val = 0x00;
> + int err = 0;
> +
> + err = regmap_read(data->regmap, AS6200_REG_CONFIG, ®_val);
> + if (err < 0)
> + return err;
> + return sprintf(buf, "%hX\n", (reg_val & AS6200_CONFIG_SS_MASK)
> + >> AS6200_CONFIG_SS_SHIFT);
> +}
> +
> +static ssize_t as6200_set_thigh(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t count)
> +{
> + int err = 0;
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct as6200_data *data = iio_priv(indio_dev);
> + struct i2c_client *client = data->client;
> + s16 reg_val;
> + int val;
> +
> + err = kstrtoint(buf, 0, &val);
> + if (err == 0) {
> + if ((val < -40) | (val > 150))
should be ||
> + return count;
> + val = (val * 10000) / 625;
> + reg_val = val << 4;
> + err = regmap_write(data->regmap, AS6200_REG_THIGH, reg_val);
> + if (err < 0)
> + return err;
> + } else
> + dev_info(&client->dev,
use dev_err if it is an error
> + "Converting value for THIGH failed, err = %hx",
> + err);
> + return count;
> +}
> +
> +static ssize_t as6200_set_tlow(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_data *data = iio_priv(indio_dev);
> + struct i2c_client *client = data->client;
> + int err = 0;
> + s16 reg_val;
> + int val;
> +
> + err = kstrtoint(buf, 0, &val);
> + if (err == 0) {
> + if ((val < -40) | (val > 150)) {
> + dev_info(&client->dev,
> + "Value for THIGH is invalid min = -40%cC, max =
> 150°C, val = %d°C",
> + (unsigned char)(248), val);
this should return an error code I think, not count
> + return count;
> + }
> + val = (val * 10000) / 625;
> + reg_val = val << 4;
> + err = regmap_write(data->regmap, AS6200_REG_TLOW, reg_val);
> + if (err < 0)
> + return err;
> + } else
> + dev_info(&client->dev,
> + "Converting value for TLOW failed, err = %hx",
> + err);
> + return count;
> +}
> +
> +static ssize_t as6200_set_thigh_reg(struct device *dev,
> + struct device_attribute *attr, const char *buf, size_t count)
> +{
> + int err = 0;
> + struct iio_dev *indio_dev = dev_to_iio_dev(dev);
> + struct as6200_data *data = iio_priv(indio_dev);
> + struct i2c_client *client = data->client;
> + u16 reg_val;
> +
> + err = kstrtou16(buf, 0, ®_val);
> + if (err == 0) {
> + err = regmap_write(data->regmap, AS6200_REG_THIGH, reg_val);
> + if (err < 0)
> + return err;
> + } else
> + dev_info(&client->dev,
> + "Converting value for THIGH failed, err = %hx",
> + err);
> + return count;
> +}
> +
> +static ssize_t as6200_set_tlow_reg(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_data *data = iio_priv(indio_dev);
> + struct i2c_client *client = data->client;
> + int err = 0;
> + u16 reg_val;
> +
> + err = kstrtou16(buf, 0, ®_val);
> + if (err == 0) {
> + err = regmap_write(data->regmap, AS6200_REG_TLOW, reg_val);
> + if (err < 0)
> + return err;
> + } else
> + dev_info(&client->dev,
> + "Converting value for TLOW failed, err = %hx",
> + err);
> + return count;
> +}
> +
> +static ssize_t as6200_set_configreg(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_data *data = iio_priv(indio_dev);
> + struct i2c_client *client = data->client;
> + int err = 0;
> + u16 reg_val;
what sense makes a driver abstraction if you allow to set any value
imaginable anyways?
try to reduce code by splitting out common functions
> +
> + err = kstrtou16(buf, 0, ®_val);
> + if (err == 0) {
> + err = regmap_write(data->regmap, AS6200_REG_CONFIG, reg_val);
> + if (err < 0)
> + return err;
> + } else
> + dev_info(&client->dev,
> + "Converting value for CONFIG failed, err = %hx",
> + err);
> + return count;
> +}
> +
drop newline
> +
> +static ssize_t as6200_set_sm(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_data *data = iio_priv(indio_dev);
> + struct i2c_client *client = data->client;
> + int err;
> + u16 val;
> +
> + err = kstrtou16(buf, 0, &val);
> + if (err == 0) {
> + err = regmap_update_bits(data->regmap, AS6200_REG_CONFIG,
> + AS6200_CONFIG_SM_MASK, val << AS6200_CONFIG_SM_SHIFT);
> + if (err < 0)
> + return err;
> + } else
> + dev_info(&client->dev,
> + "Converting value for SM field failed, err = %hx",
> + err);
> + return count;
> +}
> +
> +static ssize_t as6200_set_im(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_data *data = iio_priv(indio_dev);
> + struct i2c_client *client = data->client;
> + int err;
> + u16 val;
> +
> + err = kstrtou16(buf, 0, &val);
> + if (err == 0) {
> + err = regmap_update_bits(data->regmap, AS6200_REG_CONFIG,
> + AS6200_CONFIG_IM_MASK, val << AS6200_CONFIG_IM_SHIFT);
> + if (err < 0)
> + return err;
> + } else
> + dev_info(&client->dev,
> + "Converting value for IM field failed, err = %hx",
> + err);
> + return count;
> +}
> +
> +static ssize_t as6200_set_pol(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_data *data = iio_priv(indio_dev);
> + struct i2c_client *client = data->client;
> + int err;
> + u16 val;
> +
> + err = kstrtou16(buf, 0, &val);
> + if (err == 0) {
> + free_irq(client->irq, dev);
> + err = regmap_update_bits(data->regmap, AS6200_REG_CONFIG,
> + AS6200_CONFIG_POL_MASK,
> + val << AS6200_CONFIG_POL_SHIFT);
> + if (err < 0)
so irq is invalid here, but remove() would free it again?
> + return err;
> + as6200_setup_irq(indio_dev, false, val);
> + } else
> + dev_info(&client->dev,
> + "Converting value for POL field failed, err = %hx",
> + err);
> + return count;
> +}
> +
> +static ssize_t as6200_set_cf(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_data *data = iio_priv(indio_dev);
> + struct i2c_client *client = data->client;
> + int err;
> + u16 val;
> +
> + err = kstrtou16(buf, 0, &val);
> + if (err == 0) {
> + switch (val) {
> + case 1:
> + case 2:
> + case 4:
> + case 6:
> + err = regmap_update_bits(data->regmap,
> + AS6200_REG_CONFIG, AS6200_CONFIG_CF_MASK,
> + val << AS6200_CONFIG_CF_SHIFT);
> + if (err < 0)
> + return err;
> + break;
> + default:
> + dev_info(&client->dev,
> + "Value for CF field invalid, val = %hx", val);
> + return count;
> + }
> + } else
> + dev_info(&client->dev,
> + "Converting value for CF field failed, err = %hx",
> + err);
> + return count;
> +}
> +
> +static ssize_t as6200_set_ss(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_data *data = iio_priv(indio_dev);
> + struct i2c_client *client = data->client;
> + int err;
> + u16 val;
> +
> + err = kstrtou16(buf, 0, &val);
> + if (err == 0) {
> + err = regmap_update_bits(data->regmap, AS6200_REG_CONFIG,
> + AS6200_CONFIG_SS_MASK, val << AS6200_CONFIG_SS_SHIFT);
> + if (err < 0)
> + return err;
> + } else
> + dev_info(&client->dev,
> + "Converting value for SS field failed, err = %hx",
> + err);
> + return count;
> +}
> +
> +static IIO_DEVICE_ATTR(thigh, S_IWUSR | S_IRUGO, as6200_show_thigh,
> + as6200_set_thigh, 0);
> +static IIO_DEVICE_ATTR(tlow, S_IWUSR | S_IRUGO, as6200_show_tlow,
> + as6200_set_tlow, 0);
> +static IIO_DEVICE_ATTR(thigh_reg, S_IWUSR | S_IRUGO, as6200_show_thigh_reg,
> + as6200_set_thigh_reg, 0);
> +static IIO_DEVICE_ATTR(tlow_reg, S_IWUSR | S_IRUGO, as6200_show_tlow_reg,
> + as6200_set_tlow_reg, 0);
> +static IIO_DEVICE_ATTR(config, S_IWUSR | S_IRUGO, as6200_show_config,
> + as6200_set_configreg, 0);
> +static IIO_DEVICE_ATTR(al, S_IRUGO, as6200_show_al, NULL, 0);
> +static IIO_DEVICE_ATTR(sm, S_IWUSR | S_IRUGO, as6200_show_sm,
> + as6200_set_sm, 0);
> +static IIO_DEVICE_ATTR(im, S_IWUSR | S_IRUGO, as6200_show_im,
> + as6200_set_im, 0);
> +static IIO_DEVICE_ATTR(pol, S_IWUSR | S_IRUGO, as6200_show_pol,
> + as6200_set_pol, 0);
> +static IIO_DEVICE_ATTR(cf, S_IWUSR | S_IRUGO, as6200_show_cf,
> + as6200_set_cf, 0);
> +static IIO_DEVICE_ATTR(ss, S_IWUSR | S_IRUGO, as6200_show_ss,
> + as6200_set_ss, 0);
> +
> +static IIO_CONST_ATTR(sampling_frequency_available, "4 1 0.25 0.125");
> +
> +static struct attribute *as6200_attrs[] = {
> + &iio_dev_attr_thigh.dev_attr.attr,
> + &iio_dev_attr_tlow.dev_attr.attr,
> + &iio_dev_attr_thigh_reg.dev_attr.attr,
> + &iio_dev_attr_tlow_reg.dev_attr.attr,
> + &iio_dev_attr_config.dev_attr.attr,
> + &iio_dev_attr_al.dev_attr.attr,
> + &iio_dev_attr_sm.dev_attr.attr,
> + &iio_dev_attr_im.dev_attr.attr,
> + &iio_dev_attr_pol.dev_attr.attr,
> + &iio_dev_attr_cf.dev_attr.attr,
> + &iio_dev_attr_ss.dev_attr.attr,
> + &iio_const_attr_sampling_frequency_available.dev_attr.attr,
> + NULL,
> +};
> +
> +static struct attribute_group as6200_attr_group = {
> + .attrs = as6200_attrs,
> +};
> +
> +static const struct iio_chan_spec as6200_channels[] = {
> + {
> + .type = IIO_TEMP,
> + .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> + BIT(IIO_CHAN_INFO_PROCESSED) |
> + BIT(IIO_CHAN_INFO_SCALE),
> + .info_mask_shared_by_type = BIT(IIO_CHAN_INFO_SAMP_FREQ),
> + }
> +};
> +
> +static const struct iio_info as6200_info = {
> + .read_raw = as6200_read_raw,
> + .write_raw = as6200_write_raw,
> + .attrs = &as6200_attr_group,
> + .driver_module = THIS_MODULE,
> +};
> +
> +static int as6200_i2c_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + struct iio_dev *indio_dev;
> + struct as6200_data *data;
> + int ret;
> +
> + indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> + if (!indio_dev)
> + return -ENOMEM;
> +
> + data = iio_priv(indio_dev);
> + i2c_set_clientdata(client, indio_dev);
> + data->client = client;
> +
> + data->regmap = devm_regmap_init_i2c(client, &as6200_regmap_config);
> + if (IS_ERR(data->regmap)) {
> + ret = PTR_ERR(data->regmap);
> + dev_err(&client->dev, "regmap init failed: %d\n", ret);
> + return ret;
> + }
> +
> + indio_dev->dev.parent = &client->dev;
> + indio_dev->name = id->name;
> + indio_dev->modes = INDIO_DIRECT_MODE;
> + indio_dev->info = &as6200_info;
> +
> + indio_dev->channels = as6200_channels;
> + indio_dev->num_channels = ARRAY_SIZE(as6200_channels);
> +
> + ret = as6200_setup_irq(indio_dev, true, 0);
> + if (ret < 0)
if setup_irq() fails, irq has not been requested and no cleanup is needed
> + goto cleanup_irq;
> +
> + return iio_device_register(indio_dev);
however, if iio_device_register() fails, the irq needs to be freed
> +
> +cleanup_irq:
> + free_irq(client->irq, &client->dev);
> +
> + return ret;
> +}
> +
> +static int as6200_i2c_remove(struct i2c_client *client)
> +{
> + struct iio_dev *indio_dev = i2c_get_clientdata(client);
> +
> + iio_device_unregister(indio_dev);
> + free_irq(client->irq, &client->dev);
> + return 0;
> +}
> +
> +static const struct of_device_id as6200_of_match[] = {
> + { .compatible = "ams,as6200", },
> + {},
> +};
> +MODULE_DEVICE_TABLE(of, as6200_of_match);
> +
> +
> +static const struct i2c_device_id as6200_i2c_id[] = {
> + { "as6200", 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, as6200_id);
> +
> +static struct i2c_driver as6200_i2c_driver = {
> + .driver = {
> + .name = "as6200",
> + .owner = THIS_MODULE,
> + .of_match_table = as6200_of_match,
> + },
> + .probe = as6200_i2c_probe,
> + .remove = as6200_i2c_remove,
> + .id_table = as6200_i2c_id,
> +};
> +
> +module_i2c_driver(as6200_i2c_driver);
> +
> +MODULE_DESCRIPTION("ams AS6200 temperature sensor");
> +MODULE_AUTHOR("Elitsa Polizoeva <elitsa.polizo...@ams.com>");
> +MODULE_AUTHOR("Florian Lobmaier <florian.lobma...@ams.com>");
> +MODULE_LICENSE("GPL");
>
> -----Original Message-----
> From: Peter Meerwald-Stadler [mailto:pme...@pmeerw.net]
> Sent: Dienstag, 21. Juni 2016 15:12
> To: Florian Lobmaier <florian.lobma...@ams.com>
> Cc: ji...@kernel.org; Elitsa Polizoeva <elitsa.polizo...@ams.com>;
> knaac...@gmx.de; linux-kernel@vger.kernel.org; linux-...@vger.kernel.org;
> Lars-Peter Clausen <l...@metafoo.de>
> Subject: RE: [PATCH V1 1/1] iio: as6200: add AS6200 temperature sensor driver
> from ams AG
>
>
> > Finally we got a Datasheet which we can release and share with you.
> > Thanks for the feedback, we tried to integrate most of the feedback in
> > the driver. Some proprietary API is still present as all features of
> > the chip should be reachable. If there are any features we could use
> > from the iio framework we overlooked, please let us know.
>
> can you provide a link to the datasheet? I almost missed the attached
> document...
>
> more comments below, there still seems to be many issues; I stopped reviewing
> at some point where I noticed that previous comments have not beed addressed
>
> thanks, regards, p.
>
> > Signed-off-by: Elitsa Polizoeva <elitsa.polizo...@ams.com>
> > Signed-off-by: Florian Lobmaier <florian.lobma...@ams.com>
> > ---
> > diff --git a/drivers/iio/temperature/Kconfig
> > b/drivers/iio/temperature/Kconfig index 21feaa4..d82e80f 100644
> > --- a/drivers/iio/temperature/Kconfig
> > +++ b/drivers/iio/temperature/Kconfig
> > @@ -3,6 +3,17 @@
> > #
> > menu "Temperature sensors"
> >
> > +config AS6200
> > +tristate "ams AG AS6200 temperature sensor"
> > +depends on I2C
> > +select REGMAP_I2C
> > +help
> > + If you say yes here you get support for the AS6200 temperature
> > + sensor from ams AG.
> > +
> > + This driver can also be built as a module. If so, the module will
> > + be called as6200.
> > +
> > config MLX90614
> > tristate "MLX90614 contact-less infrared sensor"
> > depends on I2C
> > diff --git a/drivers/iio/temperature/Makefile
> > b/drivers/iio/temperature/Makefile
> > index 40710a8..c0c9a9a 100644
> > --- a/drivers/iio/temperature/Makefile
> > +++ b/drivers/iio/temperature/Makefile
> > @@ -2,5 +2,6 @@
> > # Makefile for industrial I/O temperature drivers #
> >
> > +obj-$(CONFIG_AS6200) += as6200.o
> > obj-$(CONFIG_MLX90614) += mlx90614.o
> > obj-$(CONFIG_TMP006) += tmp006.o
> > diff --git a/drivers/iio/temperature/as6200.c
> > b/drivers/iio/temperature/as6200.c
> > new file mode 100644
> > index 0000000..5e37227
> > --- /dev/null
> > +++ b/drivers/iio/temperature/as6200.c
> > @@ -0,0 +1,808 @@
>
> it is customary to put the GPL license statement here
>
> > +#include <linux/module.h>
> > +#include <linux/device.h>
> > +#include <linux/init.h>
> > +#include <linux/slab.h>
> > +#include <linux/i2c.h>
> > +#include <linux/err.h>
> > +#include <linux/types.h>
> > +#include <linux/irq.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/gpio.h>
> > +#include <linux/of_gpio.h>
> > +#include <linux/iio/iio.h>
> > +#include <linux/iio/sysfs.h>
> > +#include <linux/iio/events.h>
> > +#include <linux/regmap.h>
> > +
> > +/* to support regmap define in version 3.12 */
> > +/* supported officially in kernel 3.13 and later */
>
> please remove these comments, not relevant for mainline and not a proper
> multi-line comment
>
> > +#define regmap_reg_range(low, high) { .range_min = low, .range_max =
> > +high, }
>
> as6200_ prefix needed
>
> > +
> > +static const int as6200_conv_rates[4][2] = { {4, 0}, {1, 0}, {0,
> > +250000}, {0, 125000} };
> > +
> > +static const char * const as6200_consec_faults[] = { "1", "2", "4",
> > +"6" };
> > +
> > +/* AS6200 registers */
> > +#define AS6200_REG_TVAL0x00
> > +#define AS6200_REG_CONFIG0x01
> > +#define AS6200_REG_TLOW0x02
> > +#define AS6200_REG_THIGH0x03
> > +#define AS6200_MAX_REGISTER0x03
> > +
> > +#define AS6200_CONFIG_DW_MASK0x0010
>
> could use GENMASK() or BIT() macros to make it clearer which bits are relevant
>
> > +#define AS6200_CONFIG_DW_SHIFT4
> > +#define AS6200_CONFIG_AL_MASK0x0020
> > +#define AS6200_CONFIG_AL_SHIFT5
> > +#define AS6200_CONFIG_CR_MASK0x00C0
> > +#define AS6200_CONFIG_CR_SHIFT6
> > +#define AS6200_CONFIG_SM_MASK0x0100
> > +#define AS6200_CONFIG_SM_SHIFT8
> > +#define AS6200_CONFIG_IM_MASK0x0200
> > +#define AS6200_CONFIG_IM_SHIFT9
> > +#define AS6200_CONFIG_POL_MASK0x0400
> > +#define AS6200_CONFIG_POL_SHIFT10
> > +#define AS6200_CONFIG_CF_MASK0x1800
> > +#define AS6200_CONFIG_CF_SHIFT11
> > +#define AS6200_CONFIG_SS_MASK0x8000
> > +#define AS6200_CONFIG_SS_SHIFT15
> > +
> > +struct as6200_data {
> > +struct mutex update_lock;
>
> mutex used anywhere?
>
> > +struct i2c_client *client;
> > +int irqn;
> > +unsigned long irq_flags;
>
> irq_flags not used
>
> > +int gpio;
> > +char valid; /* !=0 if following fields are valid */ struct regmap
> > +*regmap; };
> > +
>
> drop newline
>
> > +
> > +static const struct regmap_range as6200_readable_ranges[] = {
> > +regmap_reg_range(AS6200_REG_TVAL, AS6200_REG_THIGH), };
>
> add newline (here and below: one newline please)
>
> > +static const struct regmap_access_table as6200_readable_table = {
> > +.yes_ranges = as6200_readable_ranges, .n_yes_ranges =
> > +ARRAY_SIZE(as6200_readable_ranges),
> > +};
> > +
> > +static const struct regmap_range as6200_writable_ranges[] = {
> > +regmap_reg_range(AS6200_REG_CONFIG, AS6200_REG_THIGH), }; static
> > +const struct regmap_access_table as6200_writable_table = {
> > +.yes_ranges = as6200_writable_ranges, .n_yes_ranges =
> > +ARRAY_SIZE(as6200_writable_ranges),
> > +};
> > +
> > +static const struct regmap_range as6200_volatile_ranges[] = {
> > +regmap_reg_range(AS6200_REG_TVAL, AS6200_REG_TVAL), }; static const
> > +struct regmap_access_table as6200_volatile_table = { .yes_ranges =
> > +as6200_volatile_ranges, .n_yes_ranges =
> > +ARRAY_SIZE(as6200_volatile_ranges),
> > +};
> > +static const struct regmap_config as6200_regmap_config = { .reg_bits
> > += 8, .val_bits = 16, .max_register = AS6200_MAX_REGISTER, .cache_type
> > += REGCACHE_RBTREE, .rd_table = &as6200_readable_table, .wr_table =
> > +&as6200_writable_table, .volatile_table = &as6200_volatile_table, };
> > +
> > +
> > +static int as6200_read_raw(struct iio_dev *indio_dev,
> > + struct iio_chan_spec const *channel, int *val,
> > + int *val2, long mask)
> > +{
> > +struct as6200_data *data = iio_priv(indio_dev); struct i2c_client
> > +*client = data->client; int err = 0; unsigned int reg_val = 0; int cr
> > += 0;
> > +s32 ret = 0;
>
> try to avoid unnecessary initialization
>
> > +
> > +switch (mask) {
> > +case IIO_CHAN_INFO_RAW:
> > +if (channel->type == IIO_TEMP) {
> > +err = regmap_read(data->regmap, AS6200_REG_TVAL, ®_val);
>
> error checking?
>
> > +ret = reg_val;
> > +*val = sign_extend32(ret, 15) >> 4;
> > +} else {
> > +break;
> > +}
> > +return IIO_VAL_INT;
> > +case IIO_CHAN_INFO_PROCESSED:
> > +if (channel->type == IIO_TEMP) {
> > +err = regmap_read(data->regmap, AS6200_REG_TVAL, ®_val); ret =
> > +sign_extend32(ret, 15) >> 4; *val = (ret * 625) / 10000; } else {
> > +break; } return IIO_VAL_INT;
>
> I suggest to drop _INFO_PROCESSED as _RAW + _SCALE offers equivalent
> information; _PROCESSED is not in the chan_spec?!
>
> otherwise, the code path are rather similar and could be combined
>
> > +case IIO_CHAN_INFO_SCALE:
> > +if (channel->type == IIO_TEMP) {
> > +*val = 62;
> > +*val2 = 500000;
> > +} else {
> > +break;
> > +}
> > +return IIO_VAL_INT_PLUS_MICRO;
> > +case IIO_CHAN_INFO_SAMP_FREQ:
> > +if (channel->type == IIO_TEMP) {
> > +dev_info(&client->dev, "get CR from reg");
>
> debug message, please remove
>
> > +err = regmap_read(data->regmap, AS6200_REG_CONFIG, ®_val); cr =
> > +(reg_val & AS6200_CONFIG_CR_MASK)
> > +>> AS6200_CONFIG_CR_SHIFT;
> > +*val = as6200_conv_rates[cr][0];
> > +*val2 = as6200_conv_rates[cr][1];
> > +} else {
> > +break;
> > +}
> > +return IIO_VAL_INT_PLUS_MICRO;
> > +default:
> > +break;
> > +}
> > +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_data *data = iio_priv(indio_dev); int i; unsigned int
> > +config_val;
> > +
> > +if (mask != IIO_CHAN_INFO_SAMP_FREQ)
> > +return -EINVAL;
> > +
> > +for (i = 0; i < ARRAY_SIZE(as6200_conv_rates); i++) { if ((val ==
> > +as6200_conv_rates[i][0]) &&
> > +(val2 == as6200_conv_rates[i][1])) {
> > +regmap_read(data->regmap,
> > +AS6200_REG_CONFIG, &config_val);
> > +config_val &= ~AS6200_CONFIG_CR_MASK; config_val |= i <<
> > +AS6200_CONFIG_CR_SHIFT;
> > +
> > +return regmap_write(data->regmap, AS6200_REG_CONFIG, config_val); } }
> > +return -EINVAL; }
> > +
> > +static irqreturn_t as6200_alert_isr(int irq, void *dev_id) { struct
> > +iio_dev *indio_dev = dev_id;
> > +
> > +iio_push_event(indio_dev,
> > +IIO_UNMOD_EVENT_CODE(IIO_TEMP,
> > +0,
> > +IIO_EV_TYPE_THRESH,
> > +IIO_EV_DIR_EITHER),
> > +iio_get_time_ns());
> > +return IRQ_HANDLED;
> > +}
> > +
> > +static int as6200_setup_irq(struct iio_dev *indio_dev, bool set_gpio,
> > +u8 pol) { int err; struct as6200_data *data = iio_priv(indio_dev);
> > +struct device *dev = &data->client->dev; int gpio = -1; int irq_num;
> > +int irq_trig;
> > +
> > +if (pol == 1)
> > +irq_trig = IRQF_TRIGGER_RISING;
> > +else
> > +irq_trig = IRQF_TRIGGER_FALLING;
> > +
> > +if (set_gpio) {
> > +gpio = of_get_named_gpio_flags(dev->of_node,
> > +"as6200,irq-gpio", 0, 0);
> > +err = gpio_request(gpio, "as6200_irq"); if (err) { dev_err(dev,
> > +"requesting gpio %d failed\n", gpio); return err; } err =
> > +gpio_direction_input(gpio); if (err) { dev_err(dev, "gpio %d cannot
> > +apply direction\n", gpio); return err; } }
> > +data->gpio = gpio;
> > +irq_num = gpio_to_irq(gpio);
> > +err = request_irq(irq_num, as6200_alert_isr, irq_trig, "as6200",
> > +dev); if (err) { dev_err(dev, "error requesting irq %d\n", err);
> > +return err; }
> > +data->irqn = irq_num;
> > +
> > +return 0;
> > +}
> > +
> > +static ssize_t as6200_show_thigh(struct device *dev, struct
> > +device_attribute *attr, char *buf) { struct iio_dev *indio_dev =
> > +dev_to_iio_dev(dev); struct as6200_data *data = iio_priv(indio_dev);
> > +unsigned int reg_val = 0; int err = 0; int val;
> > +
> > +err = regmap_read(data->regmap, AS6200_REG_THIGH, ®_val); reg_val
> > += reg_val >> 4; val = (625 * reg_val) / 10000; return sprintf(buf,
> > +"%d%cC\n", val, (unsigned char)(248)); }
> > +
> > +static ssize_t as6200_show_tlow(struct device *dev, struct
> > +device_attribute *attr, char *buf) { struct iio_dev *indio_dev =
> > +dev_to_iio_dev(dev); struct as6200_data *data = iio_priv(indio_dev);
> > +unsigned int reg_val = 0x00; int err = 0; int val;
> > +
> > +err = regmap_read(data->regmap, AS6200_REG_TLOW, ®_val); reg_val =
> > +reg_val >> 4; val = (625 * reg_val) / 10000; return sprintf(buf,
> > +"%d%cC\n", val, (unsigned char)(248)); }
> > +
> > +static ssize_t as6200_show_thigh_reg(struct device *dev, struct
> > +device_attribute *attr, char *buf) { struct iio_dev *indio_dev =
> > +dev_to_iio_dev(dev); struct as6200_data *data = iio_priv(indio_dev);
> > +unsigned int reg_val = 0; int err = 0;
> > +
> > +err = regmap_read(data->regmap, AS6200_REG_THIGH, ®_val); return
> > +sprintf(buf, "%hX\n", reg_val); }
> > +
> > +static ssize_t as6200_show_tlow_reg(struct device *dev, struct
> > +device_attribute *attr, char *buf) { struct iio_dev *indio_dev =
> > +dev_to_iio_dev(dev); struct as6200_data *data = iio_priv(indio_dev);
> > +unsigned int reg_val = 0x00; int err = 0;
> > +
> > +err = regmap_read(data->regmap, AS6200_REG_TLOW, ®_val); return
> > +sprintf(buf, "%hX\n", reg_val); }
> > +
> > +static ssize_t as6200_show_config(struct device *dev, struct
> > +device_attribute *attr, char *buf) { struct iio_dev *indio_dev =
> > +dev_to_iio_dev(dev); struct as6200_data *data = iio_priv(indio_dev);
> > +unsigned int reg_val = 0x00; int err = 0;
> > +
> > +err = regmap_read(data->regmap, AS6200_REG_CONFIG, ®_val); return
> > +sprintf(buf, "%hX\n", reg_val); }
> > +
> > +static ssize_t as6200_show_al(struct device *dev, struct
> > +device_attribute *attr, char *buf) { struct iio_dev *indio_dev =
> > +dev_to_iio_dev(dev); struct as6200_data *data = iio_priv(indio_dev);
> > +unsigned int reg_val = 0x00; int err = 0;
> > +u16 pol = 0;
> > +
> > +err = regmap_read(data->regmap, AS6200_REG_CONFIG, ®_val); pol =
> > +(reg_val & AS6200_CONFIG_POL_MASK) >> AS6200_CONFIG_POL_SHIFT;
> > +reg_val = (reg_val & AS6200_CONFIG_AL_MASK) >>
> > +AS6200_CONFIG_AL_SHIFT; if (pol == 1) { if (reg_val == 0) return
> > +sprintf(buf, "off\n"); else if (reg_val == 1) return sprintf(buf,
> > +"on\n"); else return sprintf(buf, "invalid\n"); } else if (pol == 0)
> > +{ if (reg_val == 0) return sprintf(buf, "on\n"); else if (reg_val ==
> > +1) return sprintf(buf, "off\n"); else return sprintf(buf,
> > +"invalid\n"); } else return sprintf(buf, "invalid\n"); }
> > +
> > +static ssize_t as6200_show_sm(struct device *dev, struct
> > +device_attribute *attr, char *buf) { struct iio_dev *indio_dev =
> > +dev_to_iio_dev(dev); struct as6200_data *data = iio_priv(indio_dev);
> > +unsigned int reg_val = 0x00; int err = 0;
> > +
> > +err = regmap_read(data->regmap, AS6200_REG_CONFIG, ®_val); return
> > +sprintf(buf, "%hX\n", (reg_val & AS6200_CONFIG_SM_MASK)
> > +>> AS6200_CONFIG_SM_SHIFT);
> > +}
> > +
> > +static ssize_t as6200_show_im(struct device *dev, struct
> > +device_attribute *attr, char *buf) { struct iio_dev *indio_dev =
> > +dev_to_iio_dev(dev); struct as6200_data *data = iio_priv(indio_dev);
> > +unsigned int reg_val = 0x00; int err = 0;
> > +
> > +err = regmap_read(data->regmap, AS6200_REG_CONFIG, ®_val); return
> > +sprintf(buf, "%hX\n", (reg_val & AS6200_CONFIG_IM_MASK)
> > +>> AS6200_CONFIG_IM_SHIFT);
> > +}
> > +
> > +static ssize_t as6200_show_pol(struct device *dev, struct
> > +device_attribute *attr, char *buf) { struct iio_dev *indio_dev =
> > +dev_to_iio_dev(dev); struct as6200_data *data = iio_priv(indio_dev);
> > +unsigned int reg_val = 0x00; int err = 0;
> > +
> > +err = regmap_read(data->regmap, AS6200_REG_CONFIG, ®_val); return
> > +sprintf(buf, "%hX\n", (reg_val & AS6200_CONFIG_POL_MASK)
> > + >> AS6200_CONFIG_POL_SHIFT);
> > +}
> > +
> > +static ssize_t as6200_show_cf(struct device *dev, struct
> > +device_attribute *attr, char *buf) { struct iio_dev *indio_dev =
> > +dev_to_iio_dev(dev); struct as6200_data *data = iio_priv(indio_dev);
> > +unsigned int reg_val = 0x00; int err = 0;
> > +
> > +err = regmap_read(data->regmap, AS6200_REG_CONFIG, ®_val); reg_val
> > += (reg_val & AS6200_CONFIG_CF_MASK) >> AS6200_CONFIG_CF_SHIFT; switch
> > +(reg_val) { case 0:
> > +return sprintf(buf, "1\n");
> > +case 1:
> > +return sprintf(buf, "2\n");
> > +case 2:
> > +return sprintf(buf, "4\n");
> > +case 3:
> > +return sprintf(buf, "6\n");
> > +}
> > +return sprintf(buf, "invalid\n");
> > +}
> > +
> > +static ssize_t as6200_show_ss(struct device *dev, struct
> > +device_attribute *attr, char *buf) { struct iio_dev *indio_dev =
> > +dev_to_iio_dev(dev); struct as6200_data *data = iio_priv(indio_dev);
> > +unsigned int reg_val = 0x00; int err = 0;
> > +
> > +err = regmap_read(data->regmap, AS6200_REG_CONFIG, ®_val); return
> > +sprintf(buf, "%hX\n", (reg_val & AS6200_CONFIG_SS_MASK)
> > +>> AS6200_CONFIG_SS_SHIFT);
> > +}
> > +
> > +static ssize_t as6200_set_thigh(struct device *dev, struct
> > +device_attribute *attr, const char *buf, size_t count) { int err = 0;
> > +struct iio_dev *indio_dev = dev_to_iio_dev(dev); struct as6200_data
> > +*data = iio_priv(indio_dev); struct i2c_client *client =
> > +data->client;
> > +s16 reg_val;
> > +int val;
> > +
> > +err = kstrtoint(buf, 0, &val);
> > +if (err == 0) {
> > +if ((val < -40) | (val > 150))
> > +return count;
> > +val = (val * 10000) / 625;
> > +reg_val = val << 4;
> > +err = regmap_write(data->regmap, AS6200_REG_THIGH, reg_val); } else
> > +dev_info(&client->dev, "Converting value for THIGH failed, err =
> > +%hx", err); return count; }
> > +
> > +static ssize_t as6200_set_tlow(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_data *data =
> > +iio_priv(indio_dev); struct i2c_client *client = data->client; int
> > +err = 0;
> > +s16 reg_val;
> > +int val;
> > +
> > +err = kstrtoint(buf, 0, &val);
> > +if (err == 0) {
> > +if ((val < -40) | (val > 150)) {
> > +dev_info(&client->dev,
> > +"Value for THIGH is invalid min = -40%cC, max = 150°C, val = %d°C",
> > +(unsigned char)(248), val); return count; } val = (val * 10000) /
> > +625; reg_val = val << 4; err = regmap_write(data->regmap,
> > +AS6200_REG_TLOW, reg_val); } else dev_info(&client->dev, "Converting
> > +value for TLOW failed, err = %hx", err); return count; }
> > +
> > +static ssize_t as6200_set_thigh_reg(struct device *dev, struct
> > +device_attribute *attr, const char *buf, size_t count) { int err = 0;
> > +struct iio_dev *indio_dev = dev_to_iio_dev(dev); struct as6200_data
> > +*data = iio_priv(indio_dev); struct i2c_client *client =
> > +data->client;
> > +u16 reg_val;
> > +
> > +err = kstrtou16(buf, 0, ®_val);
> > +if (err == 0)
> > +err = regmap_write(data->regmap, AS6200_REG_THIGH, reg_val); else
> > +dev_info(&client->dev, "Converting value for THIGH failed, err =
> > +%hx", err); return count; }
> > +
> > +static ssize_t as6200_set_tlow_reg(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_data *data =
> > +iio_priv(indio_dev); struct i2c_client *client = data->client; int
> > +err = 0;
> > +u16 reg_val;
> > +
> > +err = kstrtou16(buf, 0, ®_val);
> > +if (err == 0)
> > +err = regmap_write(data->regmap, AS6200_REG_TLOW, reg_val); else
> > +dev_info(&client->dev, "Converting value for TLOW failed, err = %hx",
> > +err); return count; }
> > +
> > +static ssize_t as6200_set_configreg(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_data *data =
> > +iio_priv(indio_dev); struct i2c_client *client = data->client; int
> > +err = 0;
> > +u16 reg_val;
> > +
> > +err = kstrtou16(buf, 0, ®_val);
> > +if (err == 0)
> > +err = regmap_write(data->regmap, AS6200_REG_CONFIG, reg_val); else
> > +dev_info(&client->dev, "Converting value for CONFIG failed, err =
> > +%hx", err); return count; }
> > +
> > +
> > +static ssize_t as6200_set_sm(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_data *data =
> > +iio_priv(indio_dev); struct i2c_client *client = data->client; int
> > +err;
> > +u16 val;
> > +
> > +err = kstrtou16(buf, 0, &val);
> > +if (err == 0) {
> > +err = regmap_update_bits(data->regmap, AS6200_REG_CONFIG,
> > +AS6200_CONFIG_SM_MASK, val << AS6200_CONFIG_SM_SHIFT); } else
> > +dev_info(&client->dev, "Converting value for SM field failed, err =
> > +%hx", err); return count; }
> > +
> > +static ssize_t as6200_set_im(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_data *data =
> > +iio_priv(indio_dev); struct i2c_client *client = data->client; int
> > +err;
> > +u16 val;
> > +
> > +err = kstrtou16(buf, 0, &val);
> > +if (err == 0) {
> > +err = regmap_update_bits(data->regmap, AS6200_REG_CONFIG,
> > +AS6200_CONFIG_IM_MASK, val << AS6200_CONFIG_IM_SHIFT); } else
> > +dev_info(&client->dev, "Converting value for IM field failed, err =
> > +%hx", err); return count; }
> > +
> > +static ssize_t as6200_set_pol(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_data *data =
> > +iio_priv(indio_dev); struct i2c_client *client = data->client; int
> > +err; int irq_num = data->irqn;
> > +u16 val;
> > +
> > +err = kstrtou16(buf, 0, &val);
> > +if (err == 0) {
> > +free_irq(irq_num, dev);
> > +err = regmap_update_bits(data->regmap, AS6200_REG_CONFIG,
> > +AS6200_CONFIG_POL_MASK, val << AS6200_CONFIG_POL_SHIFT);
> > +as6200_setup_irq(indio_dev, false, val); } else
> > +dev_info(&client->dev, "Converting value for POL field failed, err =
> > +%hx", err); return count; }
> > +
> > +static ssize_t as6200_set_cf(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_data *data =
> > +iio_priv(indio_dev); struct i2c_client *client = data->client; int
> > +err;
> > +u16 val;
> > +
> > +err = kstrtou16(buf, 0, &val);
> > +if (err == 0) {
> > +switch (val) {
> > +case 1:
> > +case 2:
> > +case 4:
> > +case 6:
> > +err = regmap_update_bits(data->regmap, AS6200_REG_CONFIG,
> > +AS6200_CONFIG_CF_MASK, val << AS6200_CONFIG_CF_SHIFT); break;
> > +default:
> > +dev_info(&client->dev,
> > +"Value for CF field invalid, val = %hx", val); return count; } } else
> > +dev_info(&client->dev, "Converting value for CF field failed, err =
> > +%hx", err); return count; }
> > +
> > +static ssize_t as6200_set_ss(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_data *data =
> > +iio_priv(indio_dev); struct i2c_client *client = data->client; int
> > +err;
> > +u16 val;
> > +
> > +err = kstrtou16(buf, 0, &val);
> > +if (err == 0) {
> > +err = regmap_update_bits(data->regmap, AS6200_REG_CONFIG,
> > +AS6200_CONFIG_SS_MASK, val << AS6200_CONFIG_SS_SHIFT); } else
> > +dev_info(&client->dev, "Converting value for SS field failed, err =
> > +%hx", err); return count; }
> > +
> > +static IIO_DEVICE_ATTR(thigh, S_IWUSR | S_IRUGO, as6200_show_thigh,
> > +as6200_set_thigh, 0); static IIO_DEVICE_ATTR(tlow, S_IWUSR | S_IRUGO,
> > +as6200_show_tlow, as6200_set_tlow, 0); static
> > +IIO_DEVICE_ATTR(thigh_reg, S_IWUSR | S_IRUGO, as6200_show_thigh_reg,
> > +as6200_set_thigh_reg, 0); static IIO_DEVICE_ATTR(tlow_reg, S_IWUSR |
> > +S_IRUGO, as6200_show_tlow_reg, as6200_set_tlow_reg, 0); static
> > +IIO_DEVICE_ATTR(config, S_IWUSR | S_IRUGO, as6200_show_config,
> > +as6200_set_configreg, 0); static IIO_DEVICE_ATTR(al, S_IRUGO,
> > +as6200_show_al, NULL, 0); static IIO_DEVICE_ATTR(sm, S_IWUSR |
> > +S_IRUGO, as6200_show_sm, as6200_set_sm, 0); static
> > +IIO_DEVICE_ATTR(im, S_IWUSR | S_IRUGO, as6200_show_im, as6200_set_im,
> > +0); static IIO_DEVICE_ATTR(pol, S_IWUSR | S_IRUGO, as6200_show_pol,
> > +as6200_set_pol, 0); static IIO_DEVICE_ATTR(cf, S_IWUSR | S_IRUGO,
> > +as6200_show_cf, as6200_set_cf, 0); static IIO_DEVICE_ATTR(ss, S_IWUSR
> > +| S_IRUGO, as6200_show_ss, as6200_set_ss, 0);
> > +
> > +static IIO_CONST_ATTR(sampling_frequency_available, "4 1 0.25
> > +0.125");
> > +
> > +static struct attribute *as6200_attrs[] = {
> > +&iio_dev_attr_thigh.dev_attr.attr,
> > +&iio_dev_attr_tlow.dev_attr.attr,
> > +&iio_dev_attr_thigh_reg.dev_attr.attr,
> > +&iio_dev_attr_tlow_reg.dev_attr.attr,
> > +&iio_dev_attr_config.dev_attr.attr,
> > +&iio_dev_attr_al.dev_attr.attr,
> > +&iio_dev_attr_sm.dev_attr.attr,
> > +&iio_dev_attr_im.dev_attr.attr,
> > +&iio_dev_attr_pol.dev_attr.attr,
> > +&iio_dev_attr_cf.dev_attr.attr,
> > +&iio_dev_attr_ss.dev_attr.attr,
> > +&iio_const_attr_sampling_frequency_available.dev_attr.attr,
> > +NULL,
> > +};
> > +
> > +static struct attribute_group as6200_attr_group = { .attrs =
> > +as6200_attrs, };
> > +
> > +static const struct iio_chan_spec as6200_channels[] = { { .type =
> > +IIO_TEMP, .info_mask_separate = BIT(IIO_CHAN_INFO_RAW) |
> > +BIT(IIO_CHAN_INFO_SCALE), .info_mask_shared_by_type =
> > +BIT(IIO_CHAN_INFO_SAMP_FREQ), } };
> > +
> > +static const struct iio_info as6200_info = { .read_raw =
> > +as6200_read_raw, .write_raw = as6200_write_raw, .attrs =
> > +&as6200_attr_group, .driver_module = THIS_MODULE, };
> > +
> > +static int as6200_i2c_of_probe(struct i2c_client *i2c, struct
> > +as6200_data *as6200) { struct device_node *np = i2c->dev.of_node;
> > +struct irq_data *irq_data;
> > +
> > +if (!np) {
> > +dev_err(&i2c->dev, "Device Tree not found\n"); return -EINVAL; }
> > +
> > +irq_data = irq_get_irq_data(i2c->irq);
>
> what is this good for?
>
> > +if (!irq_data) {
> > +dev_err(&i2c->dev, "Invalid IRQ: %d\n", i2c->irq); return -EINVAL; }
> > +
> > +as6200->irq_flags = irqd_get_trigger_type(irq_data);
> > +dev_dbg(&i2c->dev, "IRQ flags are 0x%08lx\n", as6200->irq_flags);
> > +return 0; }
> > +
> > +static int as6200_i2c_probe(struct i2c_client *client, const struct
> > +i2c_device_id *id) { struct iio_dev *indio_dev; struct as6200_data
> > +*data; int ret;
> > +
> > +indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data)); if
> > +(!indio_dev) return -ENOMEM;
> > +
> > +data = iio_priv(indio_dev);
> > +i2c_set_clientdata(client, indio_dev);
> > +data->client = client;
> > +
> > +ret = as6200_i2c_of_probe(client, data); if (ret < 0) return ret;
> > +
> > +data->regmap = devm_regmap_init_i2c(client, &as6200_regmap_config);
> > +if (IS_ERR(data->regmap)) {
> > +ret = PTR_ERR(data->regmap);
> > +dev_err(&client->dev, "regmap init failed: %d\n", ret); return ret; }
> > +
> > +indio_dev->dev.parent = &client->dev; indio_dev->name = id->name;
> > +indio_dev->modes = INDIO_DIRECT_MODE; indio_dev->info = &as6200_info;
> > +
> > +indio_dev->channels = as6200_channels; indio_dev->num_channels =
> > +ARRAY_SIZE(as6200_channels);
> > +
> > +mutex_init(&data->update_lock);
> > +ret = as6200_setup_irq(indio_dev, true, 0);
> > +
>
> no newline here
>
> > +if (ret < 0)
> > +goto cleanup_gpio_irq;
> > +
> > +return iio_device_register(indio_dev);
> > +
> > +cleanup_gpio_irq:
> > +free_irq(data->irqn, &client->dev);
> > +gpio_free(data->gpio);
> > +
> > +return ret;
> > +}
> > +
> > +static int as6200_i2c_remove(struct i2c_client *client) { struct
> > +iio_dev *indio_dev; struct as6200_data *data;
> > +
> > +indio_dev = i2c_get_clientdata(client); data = iio_priv(indio_dev);
>
> most drivers do that via initialization, not assignment for conciseness
>
> > +
> > +iio_device_unregister(indio_dev);
> > +free_irq(data->irqn, &client->dev);
> > +gpio_free(data->gpio);
> > +return 0;
> > +}
> > +
> > +static const struct of_device_id as6200_of_match[] = { { .compatible
> > += "ams,as6200", }, {}, }; MODULE_DEVICE_TABLE(of, as6200_of_match);
> > +
> > +
> > +static const struct i2c_device_id as6200_i2c_id[] = { { "as6200", 0
> > +}, { } }; MODULE_DEVICE_TABLE(i2c, as6200_id);
> > +
> > +static struct i2c_driver as6200_i2c_driver = { .driver = { .name =
> > +"as6200", .owner = THIS_MODULE, .of_match_table = as6200_of_match, },
> > +.probe = as6200_i2c_probe, .remove = as6200_i2c_remove, .id_table =
> > +as6200_i2c_id, };
> > +
> > +module_i2c_driver(as6200_i2c_driver);
> > +
> > +MODULE_DESCRIPTION("ams AS6200 temperature sensor");
> > +MODULE_AUTHOR("Elitsa Polizoeva <elitsa.polizo...@ams.com>");
> > +MODULE_AUTHOR("Florian Lobmaier <florian.lobma...@ams.com>");
> > +MODULE_LICENSE("GPL");
> >
> > -----Original Message-----
> > From: Lars-Peter Clausen [mailto:l...@metafoo.de]
> > Sent: Dienstag, 10. November 2015 12:52
> > To: Florian Lobmaier <florian.lobma...@ams.com>; ji...@kernel.org
> > Cc: Elitsa Polizoeva <elitsa.polizo...@ams.com>; knaac...@gmx.de;
> > pme...@pmeerw.net; linux-kernel@vger.kernel.org;
> > linux-...@vger.kernel.org
> > Subject: Re: [PATCH V1 1/1] iio: as6200: add AS6200 temperature sensor
> > driver from ams AG
> >
> > On 11/10/2015 10:38 AM, Florian Lobmaier wrote:
> > > The AS6200 is a compact temperature sensor chip with I2C interface.
> > >
> > > Add a driver to support the AS6200 temperature sensor.
> > >
> > > Signed-off-by: Elitsa Polizoeva <elitsa.polizo...@ams.com>
> > > Signed-off-by: Florian Lobmaier <florian.lobma...@ams.com>
> >
> > Hi,
> >
> > Thanks for the patch.
> >
> > As Peter already said this patch introduces a lot of custom ABI, none of
> > which is documented and most of which is probably not acceptable since. The
> > whole idea of a framework is that you expose the capabilities of a device
> > through a standard device independent ABI, this allows to write generic
> > applications and libraries that can manage the devices through this ABI.
> > This allows to leverage existing developments rather than starting from
> > scratch each time. If your device uses a complete custom ABI there is not
> > much point of having a driver in the first place since any application
> > needs device specific knowledge anyway to talk to the driver, in this case
> > you could just directly implement the I2C communication in the application.
> >
> > Please also consider reading and following Documentation/CodingStyle
> >
> > [...]
> > > +static int as6200_read_reg(struct i2c_client *client,
> > > +u8 reg_num, u16 *reg_val)
> > > +{
> > > +int err = 0;
> > > +char tx_buf[1];
> > > +char rx_buf[2];
> > > +
> > > +if ((reg_num >= 0) & (reg_num <= 3)) { tx_buf[0] = reg_num; err =
> > > +i2c_master_send(client, tx_buf, 1); if (err == 1) err =
> > > +i2c_master_recv(client, rx_buf, 2);
> >
> > This is not thread safe. Another thread could interrupt between
> > i2c_master_send() and i2c_master_recv() and cause undefined behavior.
> > Use
> > i2c_smbus_read_word_swapped() which makes sure that the I2C communication
> > happens atomically.
> >
> > > +if (err == 2) {
> > > +*reg_val = rx_buf[0];
> > > +*reg_val = *reg_val << 8;
> > > +*reg_val = *reg_val | rx_buf[1];
> > > +}
> > > +return err;
> > > +} else {
> > > +return -EINVAL;
> > > +}
> > > +}
> > [...]
> > > +
> > > +static irqreturn_t alert_isr(int irq, void *dev_id) {
> > > +dev_warn(dev_id, "Temperature outside of limits!");
> >
> > Please use IIO threshold events for this. Such out-of-band communication is
> > really not acceptable.
> >
> > > +return IRQ_HANDLED;
> > > +}
> > > +
> > > +static int setupIRQ(struct iio_dev *indio_dev, bool set_gpio, u8
> > > +pol) { int err; struct as6200_data *data = iio_priv(indio_dev);
> > > +struct device *dev = &data->client->dev; int gpio = -1; int
> > > +irq_num; int irq_trig;
> > > +
> > > +if (pol == 1)
> > > +irq_trig = IRQF_TRIGGER_RISING;
> > > +else
> > > +irq_trig = IRQF_TRIGGER_FALLING;
> > > +
> > > +if (set_gpio) {
> > > +gpio = of_get_named_gpio_flags(dev->of_node,
> > > +"as6200,irq-gpio", 0, 0);
> > > +err = gpio_request(gpio, "as6200_irq"); if (err) { dev_info(dev, "%s:
> > > +requesting gpio %d failed\n", as6200_id[0].name, gpio); return err;
> > > +} err = gpio_direction_input(gpio); if (err) { dev_info(dev, "%s:
> > > +gpio %d cannot apply direction\n", as6200_id[0].name, gpio); return
> > > +err; } } irq_num = gpio_to_irq(gpio); dev_info(dev, "%s:
> > > +registering for IRQ %d\n", as6200_id[0].name, irq_num);
> >
> > Please drop all these dev_info(). That's just noise in the boot log,
> > imagine every driver did this you wouldn't be able to spot the critical
> > information.
> >
> > > +err = request_irq(irq_num, alert_isr, irq_trig, as6200_id[0].name,
> > > +dev);
> >
> > Don't do all the GPIO translation. This pin is only used as a interrupt, so
> > specify it directly as an interrupt and use it that way as well.
> >
> > > +if (err) {
> > > +dev_info(dev, "%s: error requesting irq %d\n", as6200_id[0].name,
> > > +err);
> >
> > For errors use dev_err. Also the as6200_id[0].name is redundant since
> > dev_info/dev_err already prefixes the message with the device name.
> >
> > > +return err;
> > > +}
> > > +dev_info(dev, "%s: registered for IRQ %d\n", as6200_id[0].name,
> > > +irq_num); mutex_lock(&data->update_lock);
> > > +data->irqn = irq_num;
> > > +mutex_unlock(&data->update_lock);
> >
> > What exactly is protect by that mutex here?
> >
> > > +
> > > +return 0;
> > > +}
> > > +
> > [...]
> > > +if (err == 0) {
> > > +if ((val < -40) | (val > 150)) {
> > > +dev_info(&client->dev,
> > > +"Value for THIGH is invalid min = -40%cC, max = 150°C, val = %d°C",
> > > +(unsigned char)(248), val);
> >
> > Please no out-of-band error reporting.
> >
> > > +return count;
> > > +}
> > > +val = (val * 10000) / 625;
> > > +reg_val = val << 4;
> > [...]
> > > +static int as6200_probe(struct i2c_client *client, const struct
> > > +i2c_device_id *id) { struct iio_dev *indio_dev = NULL; struct
> > > +as6200_data *data = NULL;
> >
> > No need to initialize those here with dummy, this will just hide warnings
> > in case you forgot to initialize them with actual data.
> >
> > > +
> > > +indio_dev = devm_iio_device_alloc(&client->dev, sizeof(*data));
> > > +
> > > +if (!indio_dev)
> > > +return -ENOMEM;
> > > +
> > > +data = iio_priv(indio_dev);
> > > +i2c_set_clientdata(client, indio_dev);
> > > +data->client = client;
> > > +
> > > +indio_dev->dev.parent = &client->dev; indio_dev->name =
> > > +dev_name(&client->dev);
> >
> > use id->name for this. dev_name() contains things like the I2C bus and
> > device address, etc. Whereas the IIO device name should describe the type
> > of device.
> >
> > > +indio_dev->modes = INDIO_DIRECT_MODE; indio_dev->info =
> > > +&as6200_info;
> > > +
> > > +indio_dev->channels = as6200_channels; indio_dev->num_channels =
> > > +ARRAY_SIZE(as6200_channels);
> > > +
> > > +initClientData(data);
> > > +mutex_init(&data->update_lock);
> > > +setupIRQ(indio_dev, true, 0);
> > > +
> > > +return iio_device_register(indio_dev);
> >
> > Error handling is missing here, you need to free the resources that were
> > acquired in case of an error.
> >
> > > +}
> > > +
> > [...]
> >
> >
> >
>
>
--
Peter Meerwald-Stadler
+43-664-2444418 (mobile)
